[FFmpeg-devel] [PATCH v3 3/6] avformat/s337m: Make available as subdemuxer

[Tomas Härdin]

tor 2019-08-08 klockan 10:28 +0000 skrev Gaullier Nicolas:
> > > +        if (pos < size - 9 && pos >=
> > > S337M_PROBE_GUARDBAND_MIN_BYTES) 
> > I think this 9 should be an 11 or 12..
> Indeed, thank you, my mistake.
> 
> > This isn't quite what I meant by turning it into an integer test :)
> > this will likely just be rounded to zero. I meant that things could
> > likely be rearranged so there's no divisions, so the probing isn't
> > subject to the vaguaries of float >rounding
> On one side, dealing with a macro that contains a double
> (S337M_PHASE_PROBE_MIN) makes it impossible at the end to avoid a
> test involving float rounding (in my understanding, but I may be
> wrong, and I remember there are some helpers in ffmpeg to make it
> easier to avoid float rounding), or maybe I should have limited the
> digits and simply worked with an integer number of milliseconds, but
> that sounded a little bit overkill, together with the fact that the
> default value is still 0.0000 so all of this is only in case a
> developer wants to change the #define.
> On the other side, I don't think it was such a good idea to specify
> this in seconds as for the dolby_e min/max. This value is just for
> assurance that there will be no wrong probing, and I thought that
> just byte count was more appropriate... and that makes it an integer
> test at the end :)
> So I replaced : 
>   if (pos < size - 9 && 
>   (s337m_phase = (double)pos * 4 / (*st)->codecpar-
> >bits_per_coded_sample / (*st)->codecpar->sample_rate) >=
> S337M_PHASE_PROBE_MIN) {
> With just simply :
>   if (pos >= S337M_PROBE_GUARDBAND_MIN_BYTES)
> And thus replaced S337M_PHASE_PROBE_MIN=0.000 by
> S337M_PROBE_GUARDBAND_MIN_BYTES=0.
> At the end, I don't see any cons to do that, and it is far most
> simplest.

Ah, so it's time and not phase? Renaming the #defines to make that
clearer would be nice. Other than that, this isn't a huge issue

/Tomas