GNOME Bugzilla – Bug 729268
libav: avoid using wrong number of samples
Last modified: 2014-06-06 11:42:58 UTC
Created attachment 275492 [details] [review] avoid using wrong number of samples If audio_in is NULL, we'll send a NULL frame to libav, to flush the codec. In that case, we won't know how many samples the codec will have used, so we use -1 (for don't know) when letting the base class know about the buffer. I'm not 100% sure this is correct, as the base class will then try to calculate a number of samples if passed a negative value. However, a comment says it is a best guess only. So up for comments just in case. See also https://bugzilla.gnome.org/show_bug.cgi?id=724536
With -1 it assumes that one buffer is consumed or something like that. As we only send NULL in there at EOS, I think the correct answer would be "1" frame here.
It's a number of samples, not frames. Or am I misunderstanding you ? Putting 1 here would tell the base class there was 1 sample. But in fact, I'm just thinking that we should be able to tell how many samples from the data size and number of channels + bytes per sample...
Wait, it's the encoder, not the decoder, so nevermind about channel/bps... I guess we could keep track over the whole encoding of how much we supplied, and use the difference with what we accumulated from nb_frames. Not sure if that would always give the correct number though, if there is any preroll and other such things going on.
It's not giving the right number usually :) In gst-omx and androidmedia I have per-codec samples-per-frame information for this.
Fair enough. Could you explain in a few more words why you think 1 is correct here ? I don't see the reasoning.
Because I mixed encoder and decoder too. 1 is wrong :)
In fact, I think -1 is actually correct, from this gst_audio_encoder_finish_frame comment: * If @samples < 0, then best estimate is all samples provided to encoder * (subclass) so far.
-1 is wrong if the encoder has some internal latency and will output some more data at EOS, or if the encoder outputs multiple packets per input buffer, or things like that. Unfortunately the audio codec base classes are very picky about this stuff, so if the counting goes wrong at some point they will post an ERROR message.
But this patch is in the case whre you flush (!audio_in). I guess it's possible that a codec output more than one packet at EOS, though...
Oh, also if variable frame size. Hmm... :/
Though it was doing that before, so the patch changes ony the flush case.
I guess it's the best we can do for the EOS case
Comment on attachment 275492 [details] [review] avoid using wrong number of samples So let's push that for now and add a big FIXME comment above that... especially for the variable frame size thing...
Created attachment 278018 [details] [review] add the fixme
commit 17e2e9acd9b9be3cc3637a60507892ca4a97bd0a Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Apr 30 15:30:45 2014 +0100 avaudenc: avoid using wrong number of samples If audio_in is NULL, we'll send a NULL frame to libav, to flush the codec. In that case, we won't know how many samples the codec will have used, so we use -1 (for don't know) when letting the base class know about the buffer. Coverity 1195177