Skip to content

Fix parsing of G bit in VP9 scalability structure#283

Merged
Sean-Der merged 2 commits intopion:masterfrom
WonderInventions:pr-vp9-g
Feb 15, 2025
Merged

Fix parsing of G bit in VP9 scalability structure#283
Sean-Der merged 2 commits intopion:masterfrom
WonderInventions:pr-vp9-g

Conversation

@robfig
Copy link
Copy Markdown
Contributor

@robfig robfig commented Oct 30, 2024

Previously it would erroneously look at some of the reserved fields.

Previously it would erroneously look at some of the reserved fields.
@Sean-Der
Copy link
Copy Markdown
Member

Thank you @robfig sorry this took so long :(

@Sean-Der
Copy link
Copy Markdown
Member

@robfig Would you mind looking at the test TestVP9Packet_Unmarshal/ScalabilityMissingReferenceIndices it starts failing with this change.

I should have time during the holidays though! I will try to come back to this.

@robfig
Copy link
Copy Markdown
Contributor Author

robfig commented Dec 18, 2024

I just exported this fix from our own fork and don't have any particular knowledge about it, so I'll have to tag in @cptpcrd

@cptpcrd
Copy link
Copy Markdown

cptpcrd commented Dec 18, 2024

@robfig Would you mind looking at the test TestVP9Packet_Unmarshal/ScalabilityMissingReferenceIndices it starts failing with this change.

It appears that this test (and also ScalabilityMissingNG and ScalabilityMissingTemporalLayerIDs) are not actually setting the G bit - they're setting one of the reserved bits, and relying the current erroneous parsing of the G bit:

$ git diff
diff --git a/codecs/vp9_packet.go b/codecs/vp9_packet.go
index 63f05e1..2704097 100644
--- a/codecs/vp9_packet.go
+++ b/codecs/vp9_packet.go
@@ -436,6 +436,9 @@ func (p *VP9Packet) parseSSData(packet []byte, pos int) (int, error) {
        p.NS = packet[pos] >> 5
        p.Y = packet[pos]&0x10 != 0
        p.G = packet[pos]&0x8 != 0
+       if packet[pos]&0x7 != 0 {
+               panic("Reserved bits set")
+       }
        pos++
 
        NS := p.NS + 1
diff --git a/codecs/vp9_packet_test.go b/codecs/vp9_packet_test.go
index 15205ef..1f58824 100644
--- a/codecs/vp9_packet_test.go
+++ b/codecs/vp9_packet_test.go
@@ -169,21 +169,6 @@ func TestVP9Packet_Unmarshal(t *testing.T) {
                                Payload: []byte{},
                        },
                },
-               "ScalabilityStructureReserved": {
-                       b: []byte{
-                               0x0A,
-                               (1 << 5) | (0 << 4) | (0 << 3) | (1 << 2) | (1 << 1) | 1, // NS:1 Y:0 G:0, reserved fields set to 1
-                       },
-                       pkt: VP9Packet{
-                               B:       true,
-                               V:       true,
-                               NS:      1,
-                               Y:       false,
-                               G:       false,
-                               NG:      0,
-                               Payload: []byte{},
-                       },
-               },
                "ScalabilityStructure_ShortPacket0": {
                        b:   []byte{0x0A, 0x10},
                        err: errShortPacket,
$ go test ./codecs
--- FAIL: TestVP9Packet_Unmarshal (0.00s)
    --- FAIL: TestVP9Packet_Unmarshal/ScalabilityMissingReferenceIndices (0.00s)
panic: Reserved bits set [recovered]
	panic: Reserved bits set
...

I've pushed a change to the tests which fixes this.

@Sean-Der Sean-Der merged commit a78a4a6 into pion:master Feb 15, 2025
@Sean-Der
Copy link
Copy Markdown
Member

Thank you so much @robfig! Sorry it took so long. I finally have more time to work on Pion again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants