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