[PATCH] Fix for libmad audio/video sync problems (on variable bitrate audio)
Hello, It is a fix for audio/video sync problems when using '-ao mad' for videos with mp3 soundtrack encoded with variable bitrate. The problem was investigated earlier: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2006-August/045241.html A similar fix has been successfully used for a long time in a fake mp3 pass-through decoder which passed MP3 data to DSP through quite a nasty gstreamer hack on Nokia 770 to free the resources of main CPU core when playing video. This pass-through decoder initially suffered from the same a/v sync problem, so it had to be fixed. Now with the release of new firmware for Nokia N800 (IT OS2008), the situation has changed. Because of the new power management code and frequency scaling, ARM core can be clocked up to 400MHz (it was limited to 330MHz before). Unfortunately, ARM core still needs to go down to 330MHz when DSP core is active. I don't know if this problem will be fixed or not later, but right now it is more favourable to do MP3 decoding entirely on ARM core without touching DSP at all (this 70MHz bonus is more than enough to do MP3 decoding and there are still plenty of extra cpu cycles left for video). So fixed point MP3 decoders and libmad in particular became interesting again (*) and it would be very nice to get them working properly in upstream MPlayer SVN tree as well. Unfortunately, you could not confirm the audio/video sync problem with the sample I submitted earlier: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2007-September/053923.ht... I can try to reencode and upload a different sample to show the problem if you provide me with any video clip for which you are able to notice audio/video sync issues. (*) Actually, Nokia N800 has hardware floating point support, so with proper optimizations, other audio decoders might become better than fixed point decoders. But that's another story.
On Thu, 3 Jan 2008 02:52:30 +0200, Siarhei Siamashka wrote:
Hello, Unfortunately, you could not confirm the audio/video sync problem with the sample I submitted earlier: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2007-September/053923.ht...
i've heard one or two people on #mplayer say that libmad has a small desync (47ms ?)
I can try to reencode and upload a different sample to show the problem if you provide me with any video clip for which you are able to notice audio/video sync issues.
any clear video of people talking (news report) would do. -compn
On 3 January 2008, Compn wrote:
On Thu, 3 Jan 2008 02:52:30 +0200, Siarhei Siamashka wrote:
Hello, Unfortunately, you could not confirm the audio/video sync problem with the sample I submitted earlier: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2007-September/053923. html
i've heard one or two people on #mplayer say that libmad has a small desync (47ms ?)
There are not too many '-ac mad' users, otherwise there would be a lot more bugreports for sure :) Desync can vary depending on how much estimated audio bitrate (taken from the first mp3 frame) differs from the real average bitrate in the soundtrack.
I can try to reencode and upload a different sample to show the problem if you provide me with any video clip for which you are able to notice audio/video sync issues.
any clear video of people talking (news report) would do.
Actually I'm not very good at searching for videos of news reports (lost probably 20 minutes looking for them before giving up), direct link where some suitable video file could be downloaded would be very much welcome. Anyway, please try the following: $ wget ftp://upload.mplayerhq.hu/MPlayer/samples/benchmark/testsuite1/matrixbench_normdivx_vbrmp3.avi $ mencoder -oac mp3lame -lameopts br=32 -ovc lavc -lavcopts vcodec=mpeg4 -endpos 35 -o test.avi matrixbench_normdivx_vbrmp3.avi $ mplayer -ac mad test.avi $ mplayer -ac mp3 test.avi Look at the place where the policemen says "sh*t", followed by a hit sound in the next scene. Audio is a bit ahead of the picture when using libmad for audio decoding without a patch. If the desync is too small to notice, just increasing 'sh->audio_in_minsize' (from 4096 to 2*4096 for example or even to 10*4096) in 'preinit' function from 'ad_libmad.c' will make it larger. When doing more testing with larger buffer sizes, found problems with the previous patch revision (it was a quick port from a different audio decoder and unfortunately bugs slipped in). So a new patch is attached, it should be fine now. Also reformatted new code to be more consistent with the rest of the source file. Though it wasn't a primary goal, patched libmad code is even a bit faster as a side effect (older code excessively overused memmove function, shifting ~4K of data in the buffer after decoding each mp3 frame).
On Sat, 5 Jan 2008 03:10:45 +0200, Siarhei Siamashka wrote:
On 3 January 2008, Compn wrote:
On Thu, 3 Jan 2008 02:52:30 +0200, Siarhei Siamashka wrote:
Hello, Unfortunately, you could not confirm the audio/video sync problem with the sample I submitted earlier: http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2007-September/053923. html
i've heard one or two people on #mplayer say that libmad has a small desync (47ms ?)
There are not too many '-ac mad' users, otherwise there would be a lot more bugreports for sure :) Desync can vary depending on how much estimated audio bitrate (taken from the first mp3 frame) differs from the real average bitrate in the soundtrack.
I can try to reencode and upload a different sample to show the problem if you provide me with any video clip for which you are able to notice audio/video sync issues.
any clear video of people talking (news report) would do.
Actually I'm not very good at searching for videos of news reports (lost probably 20 minutes looking for them before giving up), direct link where some suitable video file could be downloaded would be very much welcome.
Anyway, please try the following:
$ wget ftp://upload.mplayerhq.hu/MPlayer/samples/benchmark/testsuite1/matrixbench_normdivx_vbrmp3.avi
$ mencoder -oac mp3lame -lameopts br=32 -ovc lavc -lavcopts vcodec=mpeg4 -endpos 35 -o test.avi matrixbench_normdivx_vbrmp3.avi
$ mplayer -ac mad test.avi
$ mplayer -ac mp3 test.avi
Look at the place where the policemen says "sh*t", followed by a hit sound in the next scene. Audio is a bit ahead of the picture when using libmad for audio decoding without a patch. If the desync is too small to notice, just increasing 'sh->audio_in_minsize' (from 4096 to 2*4096 for example or even to 10*4096) in 'preinit' function from 'ad_libmad.c' will make it larger.
When doing more testing with larger buffer sizes, found problems with the previous patch revision (it was a quick port from a different audio decoder and unfortunately bugs slipped in). So a new patch is attached, it should be fine now. Also reformatted new code to be more consistent with the rest of the source file.
Though it wasn't a primary goal, patched libmad code is even a bit faster as a side effect (older code excessively overused memmove function, shifting ~4K of data in the buffer after decoding each mp3 frame).
i'll test out libmad if i can get it to build... uoti, does this patch look correct? -compn
On Sat, 2008-01-05 at 03:10 +0200, Siarhei Siamashka wrote:
When doing more testing with larger buffer sizes, found problems with the previous patch revision (it was a quick port from a different audio decoder and unfortunately bugs slipped in). So a new patch is attached, it should be fine now. Also reformatted new code to be more consistent with the rest of the source file.
+ * Reading of audio data. This function also performs "single-shot" pts update on + * the first audio packet boundary encountered. It is done to ensure proper + * audio/video sync. This function is supposed to be called only when we have empty + * buffer with audio data and libmad decoder can't decode anything from it. + * When the buffer gets refilled, we remember 'pts' value, and advance + * 'sh->pts_bytes' as new audio data gets decoded. + */ +static int read_data(sh_audio_t *sh, unsigned char *buf, int len) +{ + mad_decoder_t *this = (mad_decoder_t *) sh->context; + int count = 0, refresh_pts_flag = 1, avail; + while (count < len) { + if (!fill_buffer(sh, &refresh_pts_flag)) return 0; + avail = len - count; + if (avail > this->buflimit - this->bufstart) avail = this->buflimit - this->bufstart; + memcpy(buf, this->bufstart, avail); + buf += avail; + this->bufstart += avail; + count += avail; + } + return count; +} Doesn't this give inaccurate pts even if the input is correctly framed? It always seems to fill the buffer completely when called so that there is likely a partial packet at the end. Then the next time it'll handle the rest of the packet, but incorrectly use the pts of the next (complete) packet. I also think it's quite ugly how it on one hand assumes some buffer information is kept in private struct fields (bufstart, buflimit) and on the other hand passes other buffer information as function parameters like "read_data(sh, &sh->a_in_buffer[sh->a_in_buffer_len], sh->a_in_buffer_size-sh->a_in_buffer_len);". That call alone is ugly too...
On 22 January 2008, Uoti Urpala wrote:
On Sat, 2008-01-05 at 03:10 +0200, Siarhei Siamashka wrote:
When doing more testing with larger buffer sizes, found problems with the previous patch revision (it was a quick port from a different audio decoder and unfortunately bugs slipped in). So a new patch is attached, it should be fine now. Also reformatted new code to be more consistent with the rest of the source file.
+ * Reading of audio data. This function also performs "single-shot" pts update on + * the first audio packet boundary encountered. It is done to ensure proper + * audio/video sync. This function is supposed to be called only when we have empty + * buffer with audio data and libmad decoder can't decode anything from it. + * When the buffer gets refilled, we remember 'pts' value, and advance + * 'sh->pts_bytes' as new audio data gets decoded. + */ +static int read_data(sh_audio_t *sh, unsigned char *buf, int len) +{ + mad_decoder_t *this = (mad_decoder_t *) sh->context; + int count = 0, refresh_pts_flag = 1, avail; + while (count < len) { + if (!fill_buffer(sh, &refresh_pts_flag)) return 0; + avail = len - count; + if (avail > this->buflimit - this->bufstart) avail = this->buflimit - this->bufstart; + memcpy(buf, this->bufstart, avail); + buf += avail; + this->bufstart += avail; + count += avail; + } + return count; +}
Doesn't this give inaccurate pts even if the input is correctly framed? It always seems to fill the buffer completely when called so that there is likely a partial packet at the end. Then the next time it'll handle the rest of the packet, but incorrectly use the pts of the next (complete) packet.
Yes, looks like you are right (audio/video sync may be one frame off). What are the options to fix it? It is probably possible to initialize sh->pts once and don't touch it unless seeking. When doing sequential decoding, sh->pts_bytes will be updated accordingly and audio/video should be properly synchronized, right? Or not? It should be mentioned, that libmad is a 'black box' for us which gets data feeded to it and produces some output. Updating pts in the middle of decoding might be not quite safe because libmad may have its own buffering issues. For example, feeding exactly one mp3 frame to libmad resulted in it returning error code MAD_ERROR_BUFLEN (not enough data in the input buffer) in my tests. A good idea, or even better - properly fixed code is welcome.
I also think it's quite ugly how it on one hand assumes some buffer information is kept in private struct fields (bufstart, buflimit) and on the other hand passes other buffer information as function parameters like "read_data(sh, &sh->a_in_buffer[sh->a_in_buffer_len], sh->a_in_buffer_size-sh->a_in_buffer_len);". That call alone is ugly too...
This 'read_data' call is a direct replacement for 'demux_read_data' call from the current code. Changing function arguments would make the patch a bit more intrusive and introduce extra changes to the code not related to its intended purpose. In addition, I don't see anything ugly here, everything is quite subjective. Struct members 'bufstart' and 'buflimit' are just the internal implementation details of this function and don't have anything common with the output data of this function (the buffer which is used as input data for libmad). Anyway, you can suggest a better solution.
On Tue, 2008-01-22 at 23:11 +0200, Siarhei Siamashka wrote:
On 22 January 2008, Uoti Urpala wrote:
Doesn't this give inaccurate pts even if the input is correctly framed? It always seems to fill the buffer completely when called so that there is likely a partial packet at the end. Then the next time it'll handle the rest of the packet, but incorrectly use the pts of the next (complete) packet.
Yes, looks like you are right (audio/video sync may be one frame off). What are the options to fix it? It is probably possible to initialize sh->pts once and don't touch it unless seeking. When doing sequential decoding, sh->pts_bytes will be updated accordingly and audio/video should be properly synchronized, right? Or not?
It'll work in the best case. However if the timestamps in the file do not exactly match the audio duration, or if the file is corrupted so that decoding of some part fails (and so you don't update pts according to that audio), then you'll lose sync with the intended pts.
It should be mentioned, that libmad is a 'black box' for us which gets data feeded to it and produces some output. Updating pts in the middle of decoding might be not quite safe because libmad may have its own buffering issues. For example, feeding exactly one mp3 frame to libmad resulted in it returning error code MAD_ERROR_BUFLEN (not enough data in the input buffer) in my tests.
Well that'll make a proper implementation harder...
A good idea, or even better - properly fixed code is welcome.
You should be able to make properly framed audio work by not fully filling the buffer. Rather read packets until some minimum amount and continue current packet beyond that up to some maximum (where the maximum is far enough away that a single packet will not hit it if it is properly framed). I don't much care whether libmad works or not so I'm not going to write that (and it took a few weeks to review the patch too...).
I also think it's quite ugly how it on one hand assumes some buffer information is kept in private struct fields (bufstart, buflimit) and on the other hand passes other buffer information as function parameters like "read_data(sh, &sh->a_in_buffer[sh->a_in_buffer_len], sh->a_in_buffer_size-sh->a_in_buffer_len);". That call alone is ugly too...
This 'read_data' call is a direct replacement for 'demux_read_data' call from the current code. Changing function arguments would make the patch a bit more intrusive and introduce extra changes to the code not related to its intended purpose.
You don't need to care about intrusiveness or avoiding extra changes - the original code is crap anyway, no need to preserve it. And since I'm not familiar with the original code nor presume it to be correct to any meaningful degree I can just as well read the new version in the patch and ignore the original.
In addition, I don't see anything ugly here, everything is quite subjective. Struct members 'bufstart' and 'buflimit' are just the internal implementation details of this function and don't have anything common with the output data of this function (the buffer which is used as input data for libmad).
'bufstart' and 'buflimit' are accessed outside the function, so they're not much more internal "to that function" than a_in_buffer. Since you're passing 'sh' to the function just use its fields directly for a_in_buffer related information too.
On 22 January 2008, Uoti Urpala wrote:
On Tue, 2008-01-22 at 23:11 +0200, Siarhei Siamashka wrote:
On 22 January 2008, Uoti Urpala wrote:
Doesn't this give inaccurate pts even if the input is correctly framed? It always seems to fill the buffer completely when called so that there is likely a partial packet at the end. Then the next time it'll handle the rest of the packet, but incorrectly use the pts of the next (complete) packet.
Yes, looks like you are right (audio/video sync may be one frame off). What are the options to fix it? It is probably possible to initialize sh->pts once and don't touch it unless seeking. When doing sequential decoding, sh->pts_bytes will be updated accordingly and audio/video should be properly synchronized, right? Or not?
It'll work in the best case. However if the timestamps in the file do not exactly match the audio duration, or if the file is corrupted so that decoding of some part fails (and so you don't update pts according to that audio), then you'll lose sync with the intended pts.
It should be mentioned, that libmad is a 'black box' for us which gets data feeded to it and produces some output. Updating pts in the middle of decoding might be not quite safe because libmad may have its own buffering issues. For example, feeding exactly one mp3 frame to libmad resulted in it returning error code MAD_ERROR_BUFLEN (not enough data in the input buffer) in my tests.
Well that'll make a proper implementation harder...
A good idea, or even better - properly fixed code is welcome.
You should be able to make properly framed audio work by not fully filling the buffer. Rather read packets until some minimum amount and continue current packet beyond that up to some maximum (where the maximum is far enough away that a single packet will not hit it if it is properly framed).
I don't much care whether libmad works or not so I'm not going to write that (and it took a few weeks to review the patch too...).
I also think it's quite ugly how it on one hand assumes some buffer information is kept in private struct fields (bufstart, buflimit) and on the other hand passes other buffer information as function parameters like "read_data(sh, &sh->a_in_buffer[sh->a_in_buffer_len], sh->a_in_buffer_size-sh->a_in_buffer_len);". That call alone is ugly too...
This 'read_data' call is a direct replacement for 'demux_read_data' call from the current code. Changing function arguments would make the patch a bit more intrusive and introduce extra changes to the code not related to its intended purpose.
You don't need to care about intrusiveness or avoiding extra changes - the original code is crap anyway, no need to preserve it. And since I'm not familiar with the original code nor presume it to be correct to any meaningful degree I can just as well read the new version in the patch and ignore the original.
In addition, I don't see anything ugly here, everything is quite subjective. Struct members 'bufstart' and 'buflimit' are just the internal implementation details of this function and don't have anything common with the output data of this function (the buffer which is used as input data for libmad).
'bufstart' and 'buflimit' are accessed outside the function, so they're not much more internal "to that function" than a_in_buffer. Since you're passing 'sh' to the function just use its fields directly for a_in_buffer related information too.
I did not forget about this patch/issue, but other more important things took my time. Also that second revision of my patch has problems with files such as ftp://upload.mplayerhq.hu/MPlayer/samples/benchmark/testsuite2/Coyote.Ugly.Sample-highbitrate-atmos.avi (just like ffmp3 decoder + default mplayer demuxer). Please disregard it. The first revision of patch, while being technically incorrect, did not have this problem and behaved better in practice (in the sense that the problems it has, are not visible to users and they do not complain) :) I'll try to come up with the third revision of patch once all the problems are resolved and it gets a reasonable time of testing by end users. I'll also provide a real reply to your post later.
participants (3)
-
Compn -
Siarhei Siamashka -
Uoti Urpala