[FFmpeg-devel] [PATCH] fix speex sample

[Michael Niedermayer]

On Thu, Apr 02, 2009 at 04:02:58PM -0700, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > On Thu, Apr 02, 2009 at 02:45:25PM -0700, Baptiste Coudurier wrote:
> >> Michael Niedermayer wrote:
> >>> On Thu, Apr 02, 2009 at 12:17:18PM -0700, Baptiste Coudurier
> >>> wrote:
> >>>> Hi
> >>>> 
> >>>> Sample testingSpeex.flv Problem is that the code above is
> >>>> setting sample rate according to the value stored, but for
> >>>> speex and nellymoser 8lhz sample rate is fixed and the values
> >>>> stored is 0.
> >>>> 
> >>>> One possibility is the to always call set_audio_codec according
> >>>> to what is stored in the packet, the function will correctly
> >>>> set sample rate for speex and nellymoser 8khz. We could also
> >>>> special case these 2 codecs.
> >>>> 
> >>>> I checked when the "if" above was added, and it seems that was
> >>>> for K70707-ARIA229.flv. I checked that with my patch, file is
> >>>> still detected correctly.
> >>>> 
> >>>> Btw Michael, if you want, I volunteer to maintain FLV code if
> >>>> it can helps, to do cleanup, simplification, and fix bugs.
> >>> i would prefer to review all changes that might change behavior
> >>> of flvdec
> >> It's up to you, but you are also the first person in charge of
> >> fixing bugs, as the flv maintainer.
> > 
> > The first job of the maintainer is to prevent more bugs from being
> > added.
> 
> Oh yeah, then you just refuse any change, so you are safe :)

ive not thought about this one but as you mention it, maybe its worth
a try, should safe some work ;)


> Very productive method, indeed ;)
> 
> > 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?


> 
> > 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


> 
> 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 ;)


> 
> 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


> 
> 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090403/78de39c2/attachment.pgp>