search

membraneframework / membrane_ffmpeg_swresample_plugin

Plugin performing audio conversion, resampling and channel mixing, using SWResample module of FFmpeg library
Apache License 2.0
2 stars 4 forks source link

Forward pts information #39

Closed dmorn closed 1 year ago

dmorn commented 1 year ago

This change forwards pts information found within input buffers to output buffers.

dmorn commented 1 year ago

Hey @mat-hek, great! I'm on it.

dmorn commented 1 year ago

Hi @mat-hek!

Therefore, I suppose we should pass PTS to swr_next_pts before each call to swr_convert and it should return the timestamp of the chunk that swr_convert is about to return

The commit I just made computes the PTS delta from the raw bytes the converter is outputting. With RawAudio.bytes_to_time we can make the convertion and compute the PTS delta as swr_next_pts is doing. That function is also dealing with compensation stuff I don't know about though. Check this out if this might be headed in the right direction for you to include!

mat-hek commented 1 year ago

Hi @dmorn, generating PTS out of the data may work in some cases, but the problem is that it doesn't reflect the incoming PTS. For example, if there's a break in the stream (because of things like network packet loss or hardware malfunction), the output PTS will just ignore that. That, in turn, may lead to losing A/V sync or other weird bugs, which already caused us a lot of trouble. I'm not sure how swr_next_pts works either, but since it accepts the input PTS, I think it should take it into account.

dmorn commented 1 year ago

@mat-hek good point, but looking at the code and the semantics of the function I don't see how it can solve it either 🧐 take a stream of packets S and compute next_pts(S(x)). Imagine that S(x+1) is lost. Now S(x+2) comes and it will get assigned the (wrong) PTS value of next_pts(S(x)). Maybe the next_pts(S(x+2)) computation is correct and S(x+3) gets a correct PTS value, but still I don't see how S(x+2) could receive a correct PTS since we compute it beforehand 🤔 Are we sure that this is the correct layer to handle packet losses? Anyway it might be better to implement it with swr_next_pts, I'm just not yet convinced it would handle the situation you're pointing out!

dmorn commented 1 year ago

BTW, if we manage to get this right we might find the correct solution for https://github.com/membraneframework/membrane_aac_fdk_plugin/pull/38 as well, as what your pointing out here is also relevant there!

dmorn commented 1 year ago

We're going to forward the AAC PTS information and ensure that at each step we have a 1:1 buffer conversion / again, not sure that this is a good approach for a wider audience.

mat-hek commented 1 year ago

I'm not sure either ;) we'll try to figure it out and reach back.

dmorn commented 1 year ago

We can discuss it at the elixir conf 😉