Fix wrong PictureID rolling over in VP9 and VP8

[jcague]

VP9 and VP8 payloads include the PictureID. This field has a length of 15 bits, but VP8.cpp and VP9.cpp were using a SeqManager with uint16_t, which is 16 bits.

This may cause different issues. I've reproduced very low framerates in VP9/SVC, but there could also be video artifacts, frozen videos, etc. VP8 also translates PictureIDs so that it could have an additional impact. The problem I saw is that we were marking all VP9/VP8 packets as isOldPacket after rolling over the PictureID. So we started to send higher temporal and spatial layers, and it broke VP9/SVC decoding.

I'm adding a bit length parameter when creating the SeqManager (SeqManager<uint16_t, 15>) to handle PictureID properly. I've added some tests for this case too.

[ibc]

Amazing. Let's review this week ASAP.

[jmillan]

Very nice!

Could you please add a test for SeqManager too?

[jcague]

I've updated how we calculate deltas inside SeqManager to fix an issue I found executing long-running tests locally. And added more unit tests to cover that case properly.

[ibc]

Just a question: somewhere we shift the payload or VP8 and VP9 (if the original received packet uses 8 bits for pictureId and the new computed pictureId value in the Consumer doesn't fit into 8 bits):

• Here we should shift the field if the new value doesn't fit into 7 bits and convert it into 16 bits and set first one to 1 to indicate that it's a 16 bits field, am I right?
• Or should we respect the size of the pictureId field in the original packet and just use 7 bits?

[jcague]

SimulcastConsumer shifts the payload in VP8 because there is a translation. But SvcConsumer doesn't translate pictureIds in VP9/SVC, so I think there's no need to shift the payload and force the two-byte header.

[ibc]

I've been told by @lminiero that "the only problem with 7-bit PictureID is that browsers often freeze video when they're used, because of a known bug, but no one uses 7-bit anymore apparently, older versions of ffmpeg were the only ones".

So I'd like to ensure what exactly we are doing when it comes to deal with 7bit pictureID.

So in VP8, we always force pictureId to be 15-bits and we properly set the first bit to 1 (so it indicates that 15 bits are used for the value). So IMHO we are good.

[ibc]

Please apply this diff in CHANGELOG.md file and IMHO we are done:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a632a5a4e..42076219a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,11 @@
 # Changelog
 
 
+### Next
+
+* Fix wrong `PictureID` rolling over in VP9 and VP8 ([PR #984(https://github.com/versatica/mediasoup/pull/984) by @jcague).
+
+
 ### 3.11.5
 
 * Require Node.js >= 16 ([PR #973(https://github.com/versatica/mediasoup/pull/973)).

[ibc]

IMHO there is nothing missing in this PR and we are good. @jmillan can we merge?

[lminiero]

So in VP8, we always force pictureId to be 15-bits and we properly set the first bit to 1 (so it indicates that 15 bits are used for the value). So IMHO we are good.

That depends on what you're doing. If you're leaving the packet as it is, setting the M bit to 1 with a PictureID that is 7 bit, means the recipient will read the following byte as well thinking it's part of the PictureID, when it isn't. This will cause errors reading the payload too, as everything will be read shifted by a byte, which will cause ugly artifacts. The only way to really force 15-bit PictureID also when using 7-bit PictureID is to shift the whole payload after that point by 1 byte, so that you reserve room for a 15-bit PictureID.

[ibc]

That depends on what you're doing. If you're leaving the packet as it is, setting the M bit to 1 with a PictureID that is 7 bit, means the recipient will read the following byte as well thinking it's part of the PictureID, when it isn't.

As per attached code screenshot above we don't do that. We always convert to 15-bits and set first bit to 1.

I mean, we never let a vp8 payload with 7-bits pictureId pass, we always convert it to 15-bits.

[lminiero]

As per attached code screenshot above we don't do that. We always convert to 15-bits and set first bit to 1.

I mean, we never let a vp8 payload with 7-bits pictureId pass, we always convert it to 15-bits.

Ack, apologies, I didn't look at the code 😁

[ibc]

Ack, apologies, I didn't look at the code 😁

Ok, you didn't look at the text either 😂

So in VP8, we always force pictureId to be 15-bits and we properly set the first bit to 1 (so it indicates that 15 bits are used for the value). So IMHO we are good.

[ibc]

Merging this. Thanks a lot @jcague

[ibc]

Released in 3.11.6.

[fippo]

but no one uses 7-bit anymore apparently

Not after @ggarber made libwebrtc emit the two-byte version unconditionally a couple of years back