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

[Kieran Clancy]

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.

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:

1393 if(len<=0) { if (!mux_a->buffer_len && ... EOF) at_eof |= 2; break; }

Then, since mux_a->buffer_len can only change from aencoder->encode()
which is not run any more, this loops forever.

For now, I think we should just fix the regression, because there are
already other users out there hitting this problem (see the post I
linked on the mplayer-users list).

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.

The problem for most variable bitrate codecs like MP3 is that it is
not actually possible to end an audio stream mid-frame. If you have
half a frame worth of audio data left at EOF, you can either discard
it (as mencoder has done), or you can pad the rest with silence. Some
may allow a special END marker, in combination with container
features, so the exact length is preserved, but MP3 does not (search
the net for "gapless mp3" to see what hackery is required in the ID3
header and client to workaround this limitation). In either case for
MP3 you are actually changing the audio stream, though the "pad with
silence" approach is arguably much better than discarding audio. It is
not trivial to fix all of mencoder's codec implementations to do this
though. Even lavc under mencoder effectively throws away stray audio
data which doesn't fit into the last frame, from what I can see.

I notice that the aencoder struct actually has a function pointer for
a close() function, which might have been the intention for this kind
of thing, but as far as I can see it is never used in any of the
mplayer/mencoder source, and none of the codecs seem to implement it
properly (in the sense of "make sure all the input data is encoded for
output with silent pad if necessary and special end markers if even
possible.")

Here's how I imagine a better solution for all of this:

When the input audio reaches EOS (which I _guess_ is when dec_audio()
returns 0, but it might do that in other weird cases like network
underrun?), execute a function aencoder->finalize(), which would need
a different implementation for each codec, and then keep requesting
output from the encoder until it stops providing data.

Thanks,
Kieran