[MPlayer-dev-eng] [PATCH] mencoder: detect end of audio stream while encoded data still buffered

[Reimar Döffinger]

On Sun, May 04, 2014 at 04:12:37AM +0930, Kieran Clancy wrote:
> On Sun, May 4, 2014 at 1:57 AM, Reimar Döffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On Sun, Apr 27, 2014 at 04:42:37PM +0930, Kieran Clancy wrote:
> >> In any case, the fix turned out to be very simple; there is no need to
> >> assume that mux_a->buffer_len is zero on end-of-stream. Even if there
> >> is outstanding data in mux_a->buffer, end-of-stream means it will
> >> never be muxed into the output because dec_audio() will not provide
> >> any more data for aencoder->encode().
> >
> > Why do you think that?
> > It could be the data is only there because the encoder just couldn't
> > encode all of it at once, so just going through the loop once more
> > (if things are done correctly) might consume some/all of it still.
> > Except that I actually don't get the relation to mp3lame at all,
> > the issue to me looks like the muxer won't accept all data,
> > so I _think_ the right solution would be to remove the condition indeed,
> > but then call muxer_write_chunk once more with a "flush" flag or so to
> > force the remaining data out.
> 
> Hi Reimar,
> 
> As I said, "mencoder lacks the architecture to request that audio
> codecs in this situation complete their partial frames on
> end-of-stream." It's not that I think this is the right (TM) way to do
> things, but it's a simple consequences of the way mencoder works and
> probably always has until recently. It would require significant
> changes across mencoder and libmpcodecs and probably other places to
> do this better.
> 
> The change made in February assumes that mux_a->buffer_len is zero on
> end-of-stream, though it is sometimes not (mp3lame just being one
> known example, but I would not be surprised if other variable bitrate
> codecs had the same problem). If you follow through the main loop
> logic, this assumption sends mencoder into an infinite loop.
> 
> Line by line, here's what happens when dec_audio() stops returning data:
> 
> 1362 len = dec_audio(...);
> 1363 if (len<=0)
> 1364 {
> 1365     len = 0;
> 1366     break;
> 1367 }
> 1368 len = aencoder->encode(...);
> 
> len is set to 0 on line 1362, and the if statement checks this and
> breaks out of the loop before the audio encoder has any chance to
> return any more frame data; not that it will, because it doesn't have
> the functionality in libmpcodecs/ae_lame.c to do this.

I still think you are looking at the completely wrong thing.
Yes, the encoder can't flush data, so some audio data will be lost.
However the data buffer_len refers to is _already encoded_ data.

> Then, when the check for len <= 0 is done on line 1393, it assumes the
> encoder has given it a whole frame already with no leftover remaining:

Since you talk about "whole frame" you seem to (I believe mistakenly?)
assume that frames have some fixed size?
Whenever we get data back from the encoder it should be a frame.
Which might mean the fact that buffer_len becomes > 0 is a bug in the
first place.

> If you wanted to go one step further, you could do a mp_msg() warning
> if mux_a->buffer_len != 0. Blindly forcing the muxer to write half an
> audio frame which the encoder hasn't finished encoding is probably not
> a good idea though.

If the encoder wasn't finished it wouldn't have returned any data, and
generally it will not be incomplete either (you could imagine encoders
that do that, but I know of none).
As such anything in the mux_a buffer definitely should be written out.