Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create av1_sample_buffer_support.go #238

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alexpokotilo
Copy link

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
Copy link
Member

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

add class to support av1 unmarshaling in SampleBuffer
use fallthrough
remove TODOs
@alexpokotilo
Copy link
Author

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
Copy link
Member

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

@alexpokotilo
Copy link
Author

alexpokotilo commented Sep 26, 2023

@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
Copy link
Member

jech commented Apr 14, 2024

@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
Copy link
Author

alexpokotilo commented Apr 15, 2024

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
Copy link
Member

jech commented Apr 15, 2024

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
Copy link
Member

jech commented Apr 15, 2024

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
Copy link
Author

alexpokotilo commented Apr 15, 2024

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
Copy link
Author

alexpokotilo commented Apr 15, 2024

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

Copy link

codecov bot commented Apr 15, 2024

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
Copy link
Member

jech commented Apr 15, 2024

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
Copy link
Member

jech commented Apr 15, 2024

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

@jech
Copy link
Member

jech commented Apr 15, 2024

Please see #264.

@alexpokotilo
Copy link
Author

Please see #264.

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

@jech
Copy link
Member

jech commented Apr 15, 2024

Please see #264 (comment) for another comment.

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