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

[Michael Niedermayer]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Incandescent light bulbs waste a lot of energy as heat so the EU forbids them.
Their replacement, compact fluorescent lamps, much more expensive, dont fit in
many old lamps, flicker, contain toxic mercury, produce a fraction of the light
that is claimed and in a unnatural spectrum rendering colors different than
in natural light. Ah and we now need to turn the heaters up more in winter to
compensate the lower wasted heat. Who wins? Not the environment, thats for sure
-------------- 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/20100912/c3a5bbe1/attachment.pgp>