fix PLI and FIR handling, wrongly triggering track.OnEnded

[EmrysMyrddin]

Description

Fix the newly added PLI and FIR handling. An error has been introduce when merging the PR.

We were reading with an empty buffer, which lead to io.ErrShortBuffer error. This was causing the RTCP read loop to crash, but the track remained sending video, since the reading and writing loops were not linked.

A potentially breaking side effect was also the call to track.OnEnded, while the track didn't realy ended in facts.

I'm fixing this by using a buffer of 1500 bytes (from what I have found in different examples), and closing the track when read errors occurs.

[codecov[bot]]

Codecov Report

Merging #420 (8501b27) into master (43272ea) will increase coverage by 0.42%. The diff coverage is 62.96%.

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
+ Coverage   47.23%   47.66%   +0.42%     
==========================================
  Files          67       67              
  Lines        4456     4469      +13     
==========================================
+ Hits         2105     2130      +25     
+ Misses       2226     2212      -14     
- Partials      125      127       +2     

Impacted Files	Coverage Δ
track.go	25.23% <62.96%> (+5.30%)	:arrow_up:
pkg/io/video/convert.go	66.66% <0.00%> (-0.33%)	:arrow_down:
pkg/io/video/convert_cgo.go	68.65% <0.00%> (+4.24%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[at-wat]

Is it possible to add a unit test?

[EmrysMyrddin]

Sure, do you have any suggestion on how to implement this tests ? Making an RTCPReader mock ?

[EmrysMyrddin]

I have implemented a unit test to check for good PLI and FIR detection. I have mocked the RTCPReader, which I think was the more easy and robust way to test this code. But the tradeoff is that the read buffer size bug would not have been detected with this test. Or at least not exactly the same bugs (the test requires a buffer size of 28 bytes minimum).

I haven't found the official maximum packet size used by pion (I'm using the size used in tests and examples). So I don't want to add a test for this.

By the way, it was necessary to add tests since the implementation was in fact not working at all. The Unmarshalling was not working but the error was ignored... I have fixed every now it should work as expected.

[at-wat]

I haven't found the official maximum packet size used by pion (I'm using the size used in tests and examples).

Maximum packet size is limited by Ethernet frame size (1500 bytes). Possible exception is Ehternet jumbo frame, but at least browsers don't use it. I'm not very sure there are any webrtc implementation using jumbo frame.

I think it's fine to use 1500 and consider jumbo frame later when requested.