[FFmpeg-devel] [PATCH] avformat/mpegts.c: reduce buffering during initialization

[Andriy Gelman]

Marton, Michael, thanks for your feedback.
I've updated the patch based on your comments.

> I can see that you are increasing linearly, maybe doubling the bufsize
> with each step is better, although in this case it probably does not
> matter too much.
> On the other hand, the maximum 8K seems small enough to allocate it at
the
> start of the function instead of reallocing it with each step.

I've allocated the buffer at the start of the function.

> You seem to be changing the logic:
> Old code: report packet size if candidate score is better than other
> scores.
> New code: report packet size with first best score no matter if the other
> scores are the same.
> Old logic seems less error-prone, so you should determine the winner
based
> on that, and stay in the loop if no winner is found.

I've changed the logic to follow the original code. The only difference is
that I've added a margin such that the winner is larger by
PROBE_PACKET_MARGIN.
The margin is set to zero when the buffer size is 8192 bytes, so that the
results of new and old code will be the same when buffer is filled, or
there is an eof.

> the end condition is not robust
> if you have scores 100 and 101, 101 is better but the difference is too
small
> to stop the loop without risk that the other might be better later

I've added a margin when comparing the scores. I.e.:
if (score > FFMAX(fec_score, dvhs_score) + margin)
        return TS_PACKET_SIZE;
Does the use of margin address your comment?

> the use of AVProbeData does not seem to help this code in any way

I've removed AVProbeData and added buffer to stack.

Regards,
Andriy


On Tue, 5 Mar 2019 at 19:25, Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Mon, Mar 04, 2019 at 10:21:01PM -0500, andriy.gelman at gmail.com wrote:
> > From: Andriy Gelman <andriy.gelman at gmail.com>
> >
> > Reduces buffering during estimation of mpegts raw_packet_size
> > parameter. Instead of buffering a fixed 8192 bytes, calculate
> > probe scores on a smaller buffer. Increase buffer size until
> > probe score is greater than minimum value.
> > ---
> >  libavformat/mpegts.c | 82 +++++++++++++++++++++++++++++++-------------
> >  1 file changed, 59 insertions(+), 23 deletions(-)
> >
> > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
> > index b04fd7b4f4..a7b33eae69 100644
> > --- a/libavformat/mpegts.c
> > +++ b/libavformat/mpegts.c
> > @@ -53,6 +53,10 @@
> >          (prev_dividend) = (dividend);
>         \
> >      } while (0)
> >
> > +#define MAX_RAW_PACKET_PROBE 8192
> > +#define PROBE_PACKET_STEP 512
> > +#define RAW_PACKET_MIN_SCORE 10
> > +
> >  enum MpegTSFilterType {
> >      MPEGTS_PES,
> >      MPEGTS_SECTION,
> > @@ -591,28 +595,64 @@ static int analyze(const uint8_t *buf, int size,
> int packet_size,
> >      return best_score - FFMAX(stat_all - 10*best_score, 0)/10;
> >  }
> >
> > -/* autodetect fec presence. Must have at least 1024 bytes  */
> > -static int get_packet_size(const uint8_t *buf, int size)
> > +/* autodetect fec presence */
> > +static int get_packet_size(AVIOContext* pb)
> >  {
> >      int score, fec_score, dvhs_score;
> > +    int pd_packet_size = TS_PACKET_SIZE;
> > +    int best_score = 0;
> > +    int ret;
> >
> > -    if (size < (TS_FEC_PACKET_SIZE * 5 + 1))
> > -        return AVERROR_INVALIDDATA;
> > +    AVProbeData pd = { 0 };
> > +    while (best_score < RAW_PACKET_MIN_SCORE &&
> > +            pd.buf_size + PROBE_PACKET_STEP <= MAX_RAW_PACKET_PROBE) {
>
> the end condition is not robust
> if you have scores 100 and 101, 101 is better but the difference is too
> small
> to stop the loop without risk that the other moght be better later
>
> the use of AVProbeData does not seem to help this code in any way
>
>
> > +
> > +        /*create extra space for next packet*/
> > +        uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size +
> PROBE_PACKET_STEP);
>
> the previous code needed no dynamic allocation. Changing that is not needed
> and would e a seperate issue and require an explanation why.
>
> overall this patch looks like it changes alot more than whats needed
> to run the code with less data in an initial iteration
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> No snowflake in an avalanche ever feels responsible. -- Voltaire
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>