[FFmpeg-devel] [PATCH] fix speex sample

[Baptiste Coudurier]

Michael Niedermayer wrote:
>>> [...]
>>>
>>> And the second is to deal with regressions like what is caused by the
>>>  last commit to flvdec.c
>> I don't think it introduced a regression. I'd like you to explain
>> though, I _will_ fix it.
>>
>> In any case you are free to revert and find a better way to _fix_ the
>> bug, but that means actually caring about it, coding, and committing ;)
> 
> which issue # at roundup is it?

Not yet in roundup, it was forwarded to me by Mike, since I added Speex
support, and I thought that fixing it straight and sending a patch was
more efficient. Roundup seems overbooked atm ;)

>>> If you want to fix bugs, that is very welcome, if you just want to
>>> flame me into steping back as flvdec maintainer so you can exchange
>>> some bugs against others i will not do so.
>> No I offered my help in maintaining aka fixing bugs _and_ regressions
>> introduced, cleaning the code (it really needs it).
> 
> i think its not that dirty compared to some other things, except maybe
> the metadata related code (not AVMeta*, i rather mean the stuff figuring
> samplerate and such out)
> and bug fixes & cleanup & ... are welcome i just like to review them

Of course, reviewing is great, I should have meant you would review the
commit instead of the patch. You never miss a commit anyway ;)

>> Then I guess you noticed how many bugs are rotting in roundup ? This is
>> becoming a nightmare and is really frightening. Nobody jumps in, and
>> this was proved by the last 2 attempts to organize a bug fixing weekend.
> 
> i know and agree, you also remember i did occassionally in the past fix a
> bunch of bugs but ATM i lack the motivation, i know thats my problem and
> i should go and bang my head against the wall ;)

Well, yes, but it's not only your problem, I believe it is our problem,
and it would be nice if everybody could participate in the effort.
I was motivated yesterday night.

>> Now I do care and I proved it, so I claim that yes some changes my
>> introduce regression but these will be fixed quickly, I engage myself.
> 
> i think the proposed fix is wrong and a correct one will not introduce a
> regression

Well, I still don't really see how and why, I've explained more below.

>> At the end I hope to get a clean, stable and maintained piece of code.
>>
>>>>> [...]
>>>>>
>>>>>> Index: libavformat/flvdec.c 
>>>>>> ===================================================================
>>>>>>  --- libavformat/flvdec.c	(revision 18316) +++
>>>>>> libavformat/flvdec.c (working copy) @@ -399,9 +399,7 @@ 
>>>>>> st->codec->sample_rate = (44100 << ((flags & 
>>>>>> FLV_AUDIO_SAMPLERATE_MASK) >> FLV_AUDIO_SAMPLERATE_OFFSET) >>
>>>>>> 3);
>>>>> the bug is in this line, this is clearly not correct for
>>>>> nelly&speex
>>>>>
>>>>> your proposed change will break flv files that really use a non 
>>>>> standard samplerate. (no i have no such file, its just
>>>>> hypothetical, but IMHO if the file says samplerate=X and X!= 0
>>>>> then this should be used)
>>>> Point is to override sample for nelly 8khz and speex, this code is 
>>>> already present in set_audio_codec.
>>>>
>>>> If you want to duplicate this code before this line, feel free to
>>>> do so.
>>>>
>>>> There is no case where storing a wrong sample would be necessary
>>>> nor supported. Specifications clearly states so.
>>> id like to support more than the spec where its natural to do so. iam
>>> not interrested in knowingly breaking other samplerates.
>> Thanks for helping and supporting the file format nightmare, I know
>> several people here will really thank you for making their life horrible.
>>
>> Why don't you support putting fourcc in TS through registration
>> descriptor, go on.
> 
> ehm
> flv has a field called "audiosamplerate", if that says 12345 ignoring it
> and explicitly using 8khz just isnt correct. thats not like adding some
> code to handle random fourccs but rather adding explicit code to reject
> some

Well this field was meant to be flexible it seems.

However flv clearly states, that codec id for nellymoser 8khz uses 8khz,
and speex use 16khz, no matter what is stored here, this value must be
ignored.

Since flv_set_audio_codec forces the correct values for these codec id,
I think that using this function is _fine_

You seem to suggest to support more than the specs but you are actually
acting _against_ the specs since specs explictly mention to _ignore_ the
value.

FLV:
"Nellymoser 8-kHz and 16-kHz are special cases-- 8- and 16-kHz sampling
rates are not supported in other formats, and the SoundRate bits can't
represent this value. When Nellymoser 8-kHz or Nellymoser 16-kHz is
specified in SoundFormat, the SoundRate and SoundType fields are
ignored. For other Nellymoser sampling rates, specify the normal
Nellymoser SoundFormat and use the SoundRate and SoundType fields as usual."
"For information regarding Speex capabilities and limitations when
stored in a SWF file, see the SWF File Format Specification."

This is not clearly stated for speex, but it does apply as well.

SWF:
"SoundRate UB[2] The sampling rate. This is
                ignored for Nellymoser and
                Speex codecs.
While Speex supports a range of sample rates, Speex audio encoded in SWF
is always encoded at 16 kHz; the SoundRate field of the DefineSound tag
is disregarded."

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org