[PATCH] mencoder: detect end of audio stream while encoded data still buffered
Changes to mencoder introduced by r36905 (2014-02-24; mencoder: Finish encoding only when audio and video reached EOF.) introduced a regression when using mp3lame and possibly other audio encoders. This was first reported by Alexander Roalter on mplayer-users: https://lists.mplayerhq.hu/pipermail/mplayer-users/2014-March/087288.html $ mencoder -msglevel all=6 -o out.avi -oac mp3lame -ovc lavc input ... (works normally until the end of the audio stream, but after the end of the video stream, an endless loop of messages begins:) ... ds_fill_buffer: EOF reached (stream: audio) ds_fill_buffer: EOF reached (stream: video) ds_fill_buffer: EOF reached (stream: audio) ds_fill_buffer: EOF reached (stream: video) ... After running a debugger, I found that the new code introduced by r36905 assumes that mux_a->buffer_len == 0 on end-of-stream, but it is often not the case for mp3lame (and probably other codecs). Due to technicalities in the way LAME works, it may generate a partial audio frame before the input stream ends, and mencoder lacks the architecture to request that audio codecs in this situation complete their partial frames on end-of-stream. I suspect other audio codecs have the same issue. 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(). Kieran Clancy --- mencoder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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.
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
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.
On Sun, May 4, 2014 at 4:31 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
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.
You are right, that sounds reasonable.
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.
I'm aware that frames have variable sizes. Perhaps there are several problems then. After the last encoded data is produced len = aencoder->encode(...); is giving > 0 bytes, but then the while(1) loop continues. Then, on a subsequent loop, len = dec_audio(...); is returning 0, overwriting the previous non-zero value of len, and preventing muxer_write_chunk(...) on line 1394 from ever seeing the data. Could the solution be as simple as something like this? Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len); This should ensure that a non-zero buffer_len will still cause one last call to muxer_write_chunk(...) on line 1394, and on the next loop through it will properly reach the EOF condition.
On Sun, May 04, 2014 at 05:19:10AM +0930, Kieran Clancy wrote:
On Sun, May 4, 2014 at 4:31 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
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.
You are right, that sounds reasonable.
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.
I'm aware that frames have variable sizes.
Perhaps there are several problems then.
After the last encoded data is produced
len = aencoder->encode(...);
is giving > 0 bytes, but then the while(1) loop continues.
Then, on a subsequent loop,
len = dec_audio(...);
is returning 0, overwriting the previous non-zero value of len, and preventing muxer_write_chunk(...) on line 1394 from ever seeing the data.
Could the solution be as simple as something like this?
Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len);
That doesn't make sense, len is the amount of data to _encode_ here, so it needs to be the amount of data in aencoder->decode_buffer. But mux_a->buffer_len is the amount of already encoded data. If we ignore the fact that we lose some amount of audio data at the end for now, the only issue is that not all data is _muxed_. There is no problem with the encoder or the amount of data it creates, gets or anything at all. I think your original patch was close to right, I just think it should call the muxing function one last time with whatever remains in mux_a->buffer_len (if anything). (That is, unless you want to fix all the other issues in the code as well, but one step at a time).
On Sun, May 4, 2014 at 7:33 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
Could the solution be as simple as something like this?
Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len);
That doesn't make sense, len is the amount of data to _encode_ here, so it needs to be the amount of data in aencoder->decode_buffer. But mux_a->buffer_len is the amount of already encoded data. If we ignore the fact that we lose some amount of audio data at the end for now, the only issue is that not all data is _muxed_. There is no problem with the encoder or the amount of data it creates, gets or anything at all. I think your original patch was close to right, I just think it should call the muxing function one last time with whatever remains in mux_a->buffer_len (if anything).
Please re-check my latest (inline) patch; it should do exactly what you describe. Since it is breaking out of the loop, len is not the amount to encode, but the amount of bytes to mux. I do admit that 'len' here is an inherently confusing variable as it's used in two completely different ways. During the loop it's used for the number of bytes decoded. Outside of the loop it's used as the number of bytes ready for muxing. It would probably be sensible to have two separate variables to avoid confusion. Kieran.
On Sun, May 4, 2014 at 5:03 AM, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
On Sun, May 4, 2014 at 7:33 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
Could the solution be as simple as something like this?
Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len);
That doesn't make sense, len is the amount of data to _encode_ here, so it needs to be the amount of data in aencoder->decode_buffer. But mux_a->buffer_len is the amount of already encoded data. If we ignore the fact that we lose some amount of audio data at the end for now, the only issue is that not all data is _muxed_. There is no problem with the encoder or the amount of data it creates, gets or anything at all. I think your original patch was close to right, I just think it should call the muxing function one last time with whatever remains in mux_a->buffer_len (if anything).
Please re-check my latest (inline) patch; it should do exactly what you describe.
Since it is breaking out of the loop, len is not the amount to encode, but the amount of bytes to mux.
I do admit that 'len' here is an inherently confusing variable as it's used in two completely different ways. During the loop it's used for the number of bytes decoded. Outside of the loop it's used as the number of bytes ready for muxing. It would probably be sensible to have two separate variables to avoid confusion.
Kieran.
I don't see a patch
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Yours truly
On Sun, May 4, 2014 at 8:58 PM, Grozdan <neutrino8@gmail.com> wrote:
Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len);
I don't see a patch
You just quoted the patch... I did say it was inline.
On Sun, May 4, 2014 at 1:39 PM, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
On Sun, May 4, 2014 at 8:58 PM, Grozdan <neutrino8@gmail.com> wrote:
Index: mencoder.c =================================================================== --- mencoder.c (revision 37181) +++ mencoder.c (working copy) @@ -1362,7 +1362,7 @@ len = dec_audio(sh_audio,aencoder->decode_buffer, aencoder->decode_buffer_size); if(len <= 0) { - len = 0; + len = mux_a->buffer_len; break; } len = aencoder->encode(aencoder, mux_a->buffer + mux_a->buffer_len, aencoder->decode_buffer, len, mux_a->buffer_size-mux_a->buffer_len);
I don't see a patch
You just quoted the patch... I did say it was inline. _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
I see it now, but didn't Reimar say it doesn't make sense a few mails back? -- Yours truly
On Sun, May 4, 2014 at 9:14 PM, Grozdan <neutrino8@gmail.com> wrote:
I see it now, but didn't Reimar say it doesn't make sense a few mails back?
Yes, so I asked him to look more closely. I still believe the patch is correct.
On 04.05.2014, at 13:46, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
On Sun, May 4, 2014 at 9:14 PM, Grozdan <neutrino8@gmail.com> wrote:
I see it now, but didn't Reimar say it doesn't make sense a few mails back?
Yes, so I asked him to look more closely. I still believe the patch is correct.
Yes, after you pointed out how it works I think it kind of is. I do think it's less obvious than it should be though, and I am not sure it doesn't have issues like flushing the encoder buffer even on decode errors instead of EOF plus I suspect the same hang bug exists in certain -oac copy cases which this patch won't fix since it only affects the encoding case.
On Sun, May 4, 2014 at 4:25 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 04.05.2014, at 13:46, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
On Sun, May 4, 2014 at 9:14 PM, Grozdan <neutrino8@gmail.com> wrote:
I see it now, but didn't Reimar say it doesn't make sense a few mails back?
Yes, so I asked him to look more closely. I still believe the patch is correct.
Yes, after you pointed out how it works I think it kind of is. I do think it's less obvious than it should be though, and I am not sure it doesn't have issues like flushing the encoder buffer even on decode errors instead of EOF plus I suspect the same hang bug exists in certain -oac copy cases which this patch won't fix since it only affects the encoding case.
Reimar, are you going to commit this patch or not? Should I wait until it's committed before pulling from SVN?
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Yours truly
On Sun, May 4, 2014 at 11:55 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
Yes, after you pointed out how it works I think it kind of is. I do think it's less obvious than it should be though, and I am not sure it doesn't have issues like flushing the encoder buffer even on decode errors instead of EOF plus I suspect the same hang bug exists in certain -oac copy cases which this patch won't fix since it only affects the encoding case.
Okay, here is a second attempt at a patch. It's a bit longer this time, but tidies up a few things, is more thorough about checking EOF (at least as thorough as the rest of the code is) and fixes any issues with other encode paths. I have added a 'bytes_ready' variable so that there is less confusion with the 'len' variable which is now just used for the decode length not the encode length. Patch description below. --- Changes to mencoder introduced by r36905 (2014-02-24; mencoder: Finish encoding only when audio and video reached EOF.) introduced a regression when using mp3lame and possibly other audio encoders. This was first reported by Alexander Roalter on mplayer-users: https://lists.mplayerhq.hu/pipermail/mplayer-users/2014-March/087288.html $ mencoder -msglevel all=6 -o out.avi -oac mp3lame -ovc lavc input ... (works normally until the end of the audio stream, but after the end of the video stream, an endless loop of messages begins:) ... ds_fill_buffer: EOF reached (stream: audio) ds_fill_buffer: EOF reached (stream: video) ds_fill_buffer: EOF reached (stream: audio) ds_fill_buffer: EOF reached (stream: video) ... After running a debugger, I found the problem is that for some VBR codecs like mp3lame, mux_a->buffer_len is not always zero on end-of-stream. This patch checks mux_a->buffer_len and ensures any remaining data is muxed. This patch also adds a new variable 'bytes_ready' to avoid using the same variable for both the number of decoded bytes AND encoded bytes. Kieran Clancy --- mencoder.c | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-)
On Mon, May 05, 2014 at 11:58:27PM +0930, Kieran Clancy wrote:
After running a debugger, I found the problem is that for some VBR codecs like mp3lame, mux_a->buffer_len is not always zero on end-of-stream.
What exactly do you mean by "for some"? For mp3lame it is this crappy workaround in ae_lame.c that should cause it: mux_a->wf->nBlockAlign=encoder->params.samples_per_frame; // required for l3codeca.acm + WMP 6.4 I would hope we have no such hacks for all others.
This patch checks mux_a->buffer_len and ensures any remaining data is muxed.
Any reason to not just do it like attached patch?
On Tue, May 6, 2014 at 2:23 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
What exactly do you mean by "for some"? For mp3lame it is this crappy workaround in ae_lame.c that should cause it: mux_a->wf->nBlockAlign=encoder->params.samples_per_frame; // required for l3codeca.acm + WMP 6.4
I would hope we have no such hacks for all others.
How does that line cause the problem? I'm not really sure what it does. It may well be in practice that mp3lame is the only codec affected. In theory though, I don't see how a VBR codec can be expected to guess in advance exactly how long its audio frame will be?
This patch checks mux_a->buffer_len and ensures any remaining data is muxed.
Any reason to not just do it like attached patch?
It makes more sense to me to do it inside the audio loop than as an addition at the end (ideally neither video nor audio frames would require flushing after their respective loops). Also, it's easily fixed but a_mux might not even be defined in the place you use it, causing a NULL pointer dereference. The other advantage of doing it inside the audio loop is that the various muxer time adjustments and status messages will be correctly updated too. If there's something you don't like about the coding style in my patch, feel free to change it, but I think it's the right place to put it. Kieran.
On 06.05.2014, at 02:01, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
On Tue, May 6, 2014 at 2:23 AM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
What exactly do you mean by "for some"? For mp3lame it is this crappy workaround in ae_lame.c that should cause it: mux_a->wf->nBlockAlign=encoder->params.samples_per_frame; // required for l3codeca.acm + WMP 6.4
I would hope we have no such hacks for all others.
How does that line cause the problem? I'm not really sure what it does.
It may well be in practice that mp3lame is the only codec affected. In theory though, I don't see how a VBR codec can be expected to guess in advance exactly how long its audio frame will be?
It can't but this line above claims that all audio frames are exactly samples_per_frame long. I am not completely sure it is this line though, it might be that the get_frame_size function instead returns something wrong.
This patch checks mux_a->buffer_len and ensures any remaining data is muxed.
Any reason to not just do it like attached patch?
It makes more sense to me to do it inside the audio loop than as an addition at the end (ideally neither video nor audio frames would require flushing after their respective loops).
Having it in the middle of the audio is in fact what I don't like, because it makes it more complex (though if I filter out the renaming I have to admit not by that much), and I think it is already more complex than it should be in the first place. Keep in mind that this is a special case for when a) we reach EOS and b) there is a buggy encoder or other issue that causes us to have data left over b) is also the reason why I think it should trigger a warning.
If there's something you don't like about the coding style in my patch, feel free to change it, but I think it's the right place to put it.
I'd at least want to split the renaming (maybe something that should be done regardless), though bytes_ready seems to generic as well, bytes_to_mux maybe is better? Maybe after that it will look simple enough to not make it worth putting it outside the loop.
On Tue, May 6, 2014 at 2:24 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 06.05.2014, at 02:01, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
It may well be in practice that mp3lame is the only codec affected. In theory though, I don't see how a VBR codec can be expected to guess in advance exactly how long its audio frame will be?
It can't but this line above claims that all audio frames are exactly samples_per_frame long. I am not completely sure it is this line though, it might be that the get_frame_size function instead returns something wrong.
When I was debugging the problem, mp3lame was returning many different frame sizes, it wasn't fixed. Is it possible that the frame size is not "wrong" but just LAME's best guess based on previous data? I just checked; ae_lame.c's get_frame_size is more-or-less just: mp_decode_mp3_header(encoder->stream->buffer); All this function does is calculate frame size from the current VBR bitrate. So LAME is choosing a bitrate based on the first chunk of audio data it receives and then get_frame_size is calculating the expected frame size. Looking at the MP3 file spec, this seems reasonable. In fact, I believe this means my first intuition was probably correct after all; this means the data in a_mux->buffer is from a incomplete frame and should not be written at all! Sure enough, when I do encodes with or without the extra bytes: mencoder summary, with first patch: Video stream: 13982.967 kbit/s (1747870 B/s) size: 8675193 bytes 4.963 secs 123 frames Audio stream: 162.319 kbit/s (20289 B/s) size: 100800 bytes 4.968 secs mencoder summary, with latest patch: Video stream: 13982.967 kbit/s (1747870 B/s) size: 8675193 bytes 4.963 secs 123 frames Audio stream: 162.152 kbit/s (20269 B/s) size: 101183 bytes 4.992 secs So we have 383 more bytes in the second audio stream, apparently. But, ffmpeg complains when processing the second file: [mp3 @ 0x150b300] incomplete frame Error while decoding stream #0:1: Invalid data found when processing input Unsurprisingly, libmpcodec's twolame and toolame implementations have the same problem. Naturally I decided to look at how lavc does things. It seems that lavc only even returns whole frames, never partial frames (so it doesn't really use the mux_a buffer "concept"). Its get_frame_size the exact size of the frame if it's ready (and 0 otherwise). I couldn't tell but I guess it has an internal buffer in which it stores excess encoded data for using in the next frame? So to me it seems the best solution, short of a massive rewrite, is to just remove the check on buffer_len exactly like my first patch does, and silently discard incomplete audio frames as has been done since the dawn of mencoder (I guess). You could put a debug level notice. As I said in my first email, it would be nice if mencoder had the architecture to request encoders complete their frames on EOS (both LAME and lavc have capabilities for padding the final frame appropriately, though this can result in slightly longer audio than the original file), but it would require big changes. Kieran
On Wed, May 07, 2014 at 01:10:25AM +0930, Kieran Clancy wrote:
On Tue, May 6, 2014 at 2:24 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 06.05.2014, at 02:01, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
It may well be in practice that mp3lame is the only codec affected. In theory though, I don't see how a VBR codec can be expected to guess in advance exactly how long its audio frame will be?
It can't but this line above claims that all audio frames are exactly samples_per_frame long. I am not completely sure it is this line though, it might be that the get_frame_size function instead returns something wrong.
When I was debugging the problem, mp3lame was returning many different frame sizes, it wasn't fixed. Is it possible that the frame size is not "wrong" but just LAME's best guess based on previous data?
I just checked; ae_lame.c's get_frame_size is more-or-less just:
mp_decode_mp3_header(encoder->stream->buffer);
All this function does is calculate frame size from the current VBR bitrate. So LAME is choosing a bitrate based on the first chunk of audio data it receives and then get_frame_size is calculating the expected frame size. Looking at the MP3 file spec, this seems reasonable.
In fact, I believe this means my first intuition was probably correct after all; this means the data in a_mux->buffer is from a incomplete frame and should not be written at all!
Oh dear. I forgot how crappy mp3lame is. That design makes no sense but they seem to have done it like that anyway. I thinking of it I remember the annoyance this caused for FFmpeg.
As I said in my first email, it would be nice if mencoder had the architecture to request encoders complete their frames on EOS (both LAME and lavc have capabilities for padding the final frame appropriately, though this can result in slightly longer audio than the original file), but it would require big changes.
I don't think so. I haven't tested it properly, but I just sent a patch that should do it. It will work fine for lavc and faac without further changes and needs only a tiny change for mp3lame. I couldn't find any documentation for toolame...
On Tue, May 6, 2014 at 8:45 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On Wed, May 07, 2014 at 01:10:25AM +0930, Kieran Clancy wrote:
On Tue, May 6, 2014 at 2:24 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
On 06.05.2014, at 02:01, Kieran Clancy <clancy.kieran+mplayer@gmail.com> wrote:
It may well be in practice that mp3lame is the only codec affected. In theory though, I don't see how a VBR codec can be expected to guess in advance exactly how long its audio frame will be?
It can't but this line above claims that all audio frames are exactly samples_per_frame long. I am not completely sure it is this line though, it might be that the get_frame_size function instead returns something wrong.
When I was debugging the problem, mp3lame was returning many different frame sizes, it wasn't fixed. Is it possible that the frame size is not "wrong" but just LAME's best guess based on previous data?
I just checked; ae_lame.c's get_frame_size is more-or-less just:
mp_decode_mp3_header(encoder->stream->buffer);
All this function does is calculate frame size from the current VBR bitrate. So LAME is choosing a bitrate based on the first chunk of audio data it receives and then get_frame_size is calculating the expected frame size. Looking at the MP3 file spec, this seems reasonable.
In fact, I believe this means my first intuition was probably correct after all; this means the data in a_mux->buffer is from a incomplete frame and should not be written at all!
Oh dear. I forgot how crappy mp3lame is. That design makes no sense but they seem to have done it like that anyway. I thinking of it I remember the annoyance this caused for FFmpeg.
As I said in my first email, it would be nice if mencoder had the architecture to request encoders complete their frames on EOS (both LAME and lavc have capabilities for padding the final frame appropriately, though this can result in slightly longer audio than the original file), but it would require big changes.
I don't think so. I haven't tested it properly, but I just sent a patch that should do it. It will work fine for lavc and faac without further changes and needs only a tiny change for mp3lame. I couldn't find any documentation for toolame...
Hi Reimar, Is this patch committed yet so I can pull a new version with the patch included? My mplayer is betting a bit old and I'd like to compile the latest SVN Thanks :)
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
-- Yours truly
On Sun, Apr 27, 2014 at 04:42:37PM +0930, Kieran Clancy wrote:
Changes to mencoder introduced by r36905 (2014-02-24; mencoder: Finish encoding only when audio and video reached EOF.) introduced a regression when using mp3lame and possibly other audio encoders.
Thanks for your efforts, I decided in the end to commit this, with a warning added for that case since with the decoder flushing implemented it _should_ not happen anymore.
participants (3)
-
Grozdan -
Kieran Clancy -
Reimar Döffinger