On Sun, May 4, 2014 at 1:57 AM, Reimar Döffinger <Reimar.Doeffinger@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