[FFmpeg-devel] [PATCH] remove out-dated ADPCM frame_size handling in libavformat

[Michael Niedermayer]

On Sun, Sep 12, 2010 at 04:10:14PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> 
> > On Sat, Sep 11, 2010 at 06:26:55PM -0400, Justin Ruggles wrote:
> >> Justin Ruggles wrote:
> >>
> >>> Michael Niedermayer wrote:
> >>>
> >>>> On Sat, Sep 11, 2010 at 11:30:07AM -0400, Justin Ruggles wrote:
> >>>>> Michael Niedermayer wrote:
> >>>>>
> >>>>>> On Wed, Sep 08, 2010 at 06:49:36PM -0400, Justin Ruggles wrote:
> >>>>>>> Justin Ruggles wrote:
> >>>>>>>
> >>>>>>>> Michael Niedermayer wrote:
> >>>>>>>>
> >>>>>>>>> On Mon, Sep 06, 2010 at 08:11:38AM -0400, Justin Ruggles wrote:
> >>>>>>>>> [...]
> >>>>>>>>>> Index: tests/ref/acodec/g726
> >>>>>>>>>> ===================================================================
> >>>>>>>>>> --- tests/ref/acodec/g726	(revision 25042)
> >>>>>>>>>> +++ tests/ref/acodec/g726	(working copy)
> >>>>>>>>>> @@ -1,4 +1,4 @@
> >>>>>>>>>> -5d8cce28f83dd33c3c7eaf43a5db5294 *./tests/data/acodec/g726.wav
> >>>>>>>>>> -24082 ./tests/data/acodec/g726.wav
> >>>>>>>>>> -4f1ba1af75dee64625a1c852e6cd01d3 *./tests/data/g726.acodec.out.wav
> >>>>>>>>>> -stddev: 8504.69 PSNR: 17.74 MAXDIFF:31645 bytes:    96104/  1058400
> >>>>>>>>>> +fd090ddf05cc3401cc75c4a5ace1d05a *./tests/data/acodec/g726.wav
> >>>>>>>>>> +24052 ./tests/data/acodec/g726.wav
> >>>>>>>>>> +74abea06027375111eeac1b2f8c7d3af *./tests/data/g726.acodec.out.wav
> >>>>>>>>>> +stddev: 8554.55 PSNR: 17.69 MAXDIFF:29353 bytes:    95984/  1058400
> >>>>>>>>> the number of samples encoded seems to be changing and not equal to
> >>>>>>>>> the input
> >>>>>>>> When the frame size in the encoder makes frames end on a byte boundary
> >>>>>>>> without any padding, the output is always identical.  Since codes are
> >>>>>>>> between 2 and 5 bits long, how would the decoder distinguish between
> >>>>>>>> padding to a byte boundary and another valid code?  I'll double-check,
> >>>>>>>> but it seems that the decoder currently treats padding as additional
> >>>>>>>> samples.
> >>>>>>> I've confirmed that this is the cause of the difference.  The parameters
> >>>>>>> used by the regression test give a 4-bit code size.  When the frame size
> >>>>>>> is odd, that leads to 1 extra sample being decoded by the decoder
> >>>>>>> because of padding.  In the current version, because of resampling from
> >>>>>>> 44100 Hz to 8000 Hz, the frame size actually varies from frame-to-frame.
> >>>>>>>
> >>>>>>> Current:
> >>>>>>> source samples             = 264600
> >>>>>>> resampled samples          =  47991
> >>>>>>> number of odd-sized frames =     61
> >>>>>>> decoded samples            =  48052
> >>>>>>> decoded data bytes         =  96104
> >>>>>>>
> >>>>>>> Patched:
> >>>>>>> source samples             = 264600
> >>>>>>> resampled samples          =  47991
> >>>>>>> number of odd-sized frames =      1 (the last frame)
> >>>>>>> decoded samples            =  47992
> >>>>>>> decoded data bytes         =  95984
> >>>>>>>
> >>>>>>> So choosing a frame size that forces the encoder to only use padding for
> >>>>>>> the last frame (which this patch does) seems to be the appropriate thing
> >>>>>>> to do.
> >>>>>> the patch is ok then
> >>>>>> the regression test still is completely broken though because it does not
> >>>>>> seem to compare files of equal sampling rate if my guess is correctly
> >>>>> Would it be better to resample back to 2-channel 44100 Hz during
> >>>>> decoding
> >>>> imho yes
> >>> ok. that is a simple fix.
> >>>
> >>> But it's not so simple for pcm_s24daud because FFmpeg can upmix from 2
> >>> to 6 channels for encoding, but it can't downmix from 6 to 2 channels
> >>> for decoding.  Should that decoding test be disabled?
> >> Or we can fix both tests by creating multiple reference files like in
> >> the attached patch.  My shell scripting skills are minimal, so there
> >> might be a simpler way to do the same thing...
> > 
> > well, doing it this way means the upmix&downmix isnt tested, it also means
> > that resampling isnt tested.
> > So seperate tests would be needed for these and the reference file used should
> > in this case not be generated by resampling but by creating a seperate
> > reference file
> > that said the idea feels like a hack, why dont you implement 6->2 downmix?
> > That is needed anyway and would avoid adding more complexity to the already
> > completely unreadable and undocumented scripts we have
> 
> It feels much more logical to me for the g726 regression test to compare
> the input to the encoder with the output from the decoder, not to
> compare resampled output to pre-resampled input.  If we want to test the
> resampling, we should have a resample test.
> 
> Creating multiple reference files from audiogen rather than resampling
> the 1 reference file is fine with me.  I made a quick patch to add
> commandline parameters to audiogen for sample rate and number of
> channels.  I just couldn't figure out how to use it cleanly in the
> Makefile and regression test scripts.
> 
> -Justin
> 
> 

>  audiogen.c |   72 ++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 27 deletions(-)
> fb205557529318927bf73b9fc3a9d1d505c66599  audiogen_params.patch

ok if tested
[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is not what we do, but why we do it that matters.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100913/a9cf1f3a/attachment.pgp>