Create av1_sample_buffer_support.go

[alexpokotilo]

add class to support av1 unmarshaling in SampleBuffer

Description

Now AV1Packet cannot be used with Sample buffer. I implemented new class for av1 assembling with SampleBuffer.
Works fine for receiving AV1 from Chrome and returning AV bitstream

Reference issue

Fixes #189

[Sean-Der]

Great work @alexpokotilo!

Mind writing some unit tests for this? I just want to make sure some of the slice indexing can't cause a panic.

thank you

[alexpokotilo]

Mind writing some unit tests for this? I just want to make sure some of the slice indexing can't cause a panic.

Hi @Sean-Der!
I've added two unit tests into av1_sample_buffer_support_test.go. First test is for AV1 OBUs with payload. The second one is for OBUs without payload aka header-only OBU. Tests checks obu size up to 3 bytes to make sure obu size math works fine.
Slice indexing made fine to me.

[Sean-Der]

🔥 great work @alexpokotilo! I will let CI run and if that passes I will merge

[alexpokotilo]

@Sean-Der I moved my files to prevent cyclic dependencies.
The only problem is that I had to add github.com/pion/webrtc/v3/pkg/media/samplebuilder as dependency to rtp project because you asked to add tests. But SampleBuffer is located at github.com/pion/webrtc/v3/pkg/media/samplebuilder so I had to add it as dependency. I don't know where to move tests to both remove github.com/pion/webrtc/v3/pkg/media/samplebuilder from rtp repo but still add tests you requested.

I can add something simple to emulate SampleBuffer behavior into this test to remove dependency from rtp repo.
What would you suggest ?

[jech]

@alexpokotilo Could you please explain why this code is necessary? Why is it not enough to remove the aggregation headers and concatenate the resulting payloads?

[alexpokotilo]

Hi @jech, thanks for the question!
av1 payload in rtp packet described by https://aomediacodec.github.io/av1-rtp-spec/#44-av1-aggregation-header and for non-webrtc applications av1 stored and processed in plain OBUs list format, without rtp things.
So if you get av1 from webrtc and process it in non-webrtc applications you cannot just assemble rtp packet. you need plain OBUs list.
Moreover to use av1 with samplebuilder.SampleBuilder I need to implement something to produce av1.
With provided class I can do

			videoStreamBuilder = samplebuilder.New(400, &AV1PacketSampleBufferSupport{}, 90000,
				samplebuilder.WithMaxTimeDelay(time.Millisecond*400))

Current codecs.AV1Packet{} implementation doesn't have isPartionHead and isPartionHead to be used with samplebuilder.

Without samplebuilder from another hand we cannot just clue av1 rtp chanks because of jitter. but samplebuilder helps us with it.
So like my branch's name says I need this class to add av1 support into samplebuffer. sample buffer from another hand requires isPartionHead and isPartionHead and doesn't work with rtp payload types but rather transport types.

Check codecs.H264Packet implementation as example. h264 payload returned in annexB form but not in rtp form.
This is why I produce plain OBUs list and implement isPartionHead and isPartionHead to use samplebuilder so that av1 packets could come in reordered form and samplebuilder withh sort them, wait for NACK is loss exists.

[jech]

Could you please explain why this code is necessary? Why is it not enough to remove the aggregation headers and concatenate the resulting payloads?

So if you get av1 from webrtc and process it in non-webrtc applications you cannot just assemble rtp packet. you need plain OBUs list.

I understand that. Please correct me if I'm wrong, however, I don't think you need to create a list of OBUs, I think you can build the packet on the fly.

Suppose you take the incoming RTP packets, reorder them, and take the sequence of packets until you find one with the mark bit set. Then you remove the aggregation headers, concatenate the rest of the bodies. You get exactly the sequence of concatenated OBUs, no? With no need to allocate a list of OBUs?

So the only change in the Samplebuilder is this line:

https://github.com/jech/samplebuilder/blob/6cbba09fc1c94ce7d82418c68e32a64a24fe860b/samplebuilder.go#L443

which will need to be modified to remove the aggregation header.

Or am I missing something?

[jech]

This could be implemented by adding an interface

interface PacketAppender {
    []byte appendPacket([]byte, []byte)
}

and in samplebuilder do something like

appender, ok := codec.(PacketAppender)
if ok {
    data = codec.appendPacket(data, buf)
} else {
    data = append(data, buf)

PacketAppender would only need to be implemented for AV1.

[alexpokotilo]

Or am I missing something?

To create sample builder we need to call samplebuilder.New and you have to provide depacketizer rtp.Depacketizer for codec you want to depacketize. We don't have any rtp.Depacketizer implementation for Av1. So my version of rtp.Depacketizer for Av1 is AV1PacketSampleBufferSupport.

When func (s *SampleBuilder) Pop() called and it calls buildSample where Unmarshal is called.
Depacketizer' interface Unmarshal method has signature Unmarshal(packet []byte) ([]byte, error)
So I have to produce one array with payload for given av1 frame to fullfill samplebuilder interface.
Again AV1PacketSampleBufferSupport is av1 implementation to be used with samplebuilder.

Suppose you take the incoming RTP packets, reorder them, and take the sequence of packets until you find one with the mark bit set. Then you remove the aggregation headers, concatenate the rest of the bodies. You get exactly the sequence of concatenated OBUs, no? With no need to allocate a list of OBUs?

Please take a look at func (p *AV1Packet) Unmarshal

There is is a comment "// If W bit is set the last OBU Element will have no length header" so I don't think OBU list is produced by just removing aggregation header. The last OBU doesn't have length at all and OBU list will be corrupted if you go your way.
Maybe I'm wrong and don't get something instead :) But I tried to do this and failed.

In my approach I use frame.AV1 to do the job. I'm not sure I do it 100% right. I just added working method to use samplebuffer and av1 which didn't exist.

[alexpokotilo]

This could be implemented by adding an interface

interface PacketAppender {
    []byte appendPacket([]byte, []byte)
}

and in samplebuilder do something like

appender, ok := codec.(PacketAppender)
if ok {
    data = codec.appendPacket(data, buf)
} else {
    data = append(data, buf)

PacketAppender would only need to be implemented for AV1.

the problem here is that there is no av1 depackager implementing rtp.Depacketizer interface and this is why I added my class. you cannot just implement new PacketAppender interface without adding something to fulfill rtp.Depacketizer for av1.
And I don't understand what you new appendPacket should do. Why we should change sample buffer interface for one given codec ? s.depacketizer.Unmarshal call should return something we could just append. and this is exactly what
func (d *AV1PacketSampleBufferSupport) Unmarshal do.

You will not be able just cut header because the last OBU in rtp doesn't have size

[codecov]

Codecov Report

Attention: Patch coverage is 79.36508% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 85.97%. Comparing base (74a9dc7) to head (59359f2).

Files	Patch %	Lines
codecs/av1/frame/av1_sample_buffer_support.go	81.81%	7 Missing and 3 partials ⚠️
codecs/av1/obu/leb128.go	62.50%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   86.21%   85.97%   -0.25%     
==========================================
  Files          22       23       +1     
  Lines        1734     1797      +63     
==========================================
+ Hits         1495     1545      +50     
- Misses        203      211       +8     
- Partials       36       41       +5     

Flag	Coverage Δ
go	85.97% <79.36%> (-0.25%)	⬇️
wasm	85.25% <71.42%> (-0.51%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jech]

I'm sure you're right and I'm wrong, but I still don't understand why. Perhaps you could give a concrete example where appending the packet bodies (with the aggregation headers removed) would not work?

And I don't understand what you new appendPacket should do.

It would remove the aggregation header and append the rest of the packet to the given buffer.

[jech]

Folks, please refrain from merging this commit until Monday, I want to work out why I'm wrong.

[jech]

Please see #264.

[alexpokotilo]

Please see #264.

Great!
I'll try to use av1_packet.go with your fixes and let you know

[jech]

Please see #264 (comment) for another comment.