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@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.