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

[Justin Ruggles]

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


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: audiogen_params.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100912/426b3d28/attachment.asc>