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

Submitted by Andriy Gelman on March 5, 2019, 3:21 a.m.

Details

Message ID 20190305032101.18661-1-andriy.gelman@gmail.com
State New
Headers show

Commit Message

Andriy Gelman March 5, 2019, 3:21 a.m.
From: Andriy Gelman <andriy.gelman@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(-)

Comments

Marton Balint March 5, 2019, 4:59 p.m.
On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote:

> From: Andriy Gelman <andriy.gelman@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.

What is the issue that you are seeing and that this patch fix? Is it only 
matters for low bitrate mpegts? (like 64 kbps streams?) Or does this 
improve high bitrate streams as well somehow?

Thanks,
Marton
Andriy Gelman March 5, 2019, 8:58 p.m.
> What is the issue that you are seeing and that this patch fix? Is it only
> matters for low bitrate mpegts? (like 64 kbps streams?) Or does this
> improve high bitrate streams as well somehow?

I'm muxing hevc video + very low bitrate binary messages into mpegts.
If the hevc video is not present, I would have to buffer 8192 bytes of
binary messages to get past mpegts_read_header, which was creating a large
latency.
This patch reduces the startup time. As you mentioned, it does not apply to
high bitrate streams.

Also in git history, I saw that the size of the buffer was changed in
ad-hoc way from 1024 -> 5*1024-> 8 * 1024 because I assume probing failed
in some cases.
I feel that the proposed patch would be a better solution as it follows the
same ideas when probing for unknown parameters in other parts of the code.

Regards,
Andriy


On Tue, 5 Mar 2019 at 11:59, Marton Balint <cus@passwd.hu> wrote:

>
>
> On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote:
>
> > From: Andriy Gelman <andriy.gelman@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.
>
> What is the issue that you are seeing and that this patch fix? Is it only
> matters for low bitrate mpegts? (like 64 kbps streams?) Or does this
> improve high bitrate streams as well somehow?
>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Marton Balint March 6, 2019, 12:06 a.m.
On Mon, 4 Mar 2019, andriy.gelman@gmail.com wrote:

> From: Andriy Gelman <andriy.gelman@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.

Please extend the commit message with the justification for this change,
e.g. "reduces buffering latency with low bitrate streams, where 8192 bytes 
can mean several seconds"

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

The initial value can be 0 or AVERROR_INVALIDDATA, because calling code 
falls back to this anyway and reports the issue.

> +    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) {
> +
> +        /*create extra space for next packet*/
> +        uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + PROBE_PACKET_STEP);
> +        if (new_buf) {
> +            pd.buf = new_buf;
> +            ret = avio_read(pb, pd.buf + pd.buf_size, PROBE_PACKET_STEP);

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.


> +            if (ret < 0) {
> +                av_log(pb, AV_LOG_ERROR, "Error reading from input: %s.\n",
> +                   av_err2str(ret));

This probably should not report an error for AVERROR_EOF, mpegts files 
less than 8192 bytes can be valid.

> +                break;
> +            }
> +            pd.buf_size += ret;
> +        } else
> +            goto fail;
> +
> +        /*check score for each fec packet size*/
> +        score      = analyze(pd.buf, pd.buf_size, TS_PACKET_SIZE,      0);
> +        if (score > best_score) {
> +            best_score = score;
> +            pd_packet_size = TS_PACKET_SIZE;
> +        }
> 
> -    score      = analyze(buf, size, TS_PACKET_SIZE,      0);
> -    dvhs_score = analyze(buf, size, TS_DVHS_PACKET_SIZE, 0);
> -    fec_score  = analyze(buf, size, TS_FEC_PACKET_SIZE,  0);
> -    av_log(NULL, AV_LOG_TRACE, "score: %d, dvhs_score: %d, fec_score: %d \n",
> -            score, dvhs_score, fec_score);
> -
> -    if (score > fec_score && score > dvhs_score)
> -        return TS_PACKET_SIZE;
> -    else if (dvhs_score > score && dvhs_score > fec_score)
> -        return TS_DVHS_PACKET_SIZE;
> -    else if (score < fec_score && dvhs_score < fec_score)
> -        return TS_FEC_PACKET_SIZE;
> -    else
> -        return AVERROR_INVALIDDATA;
> +        dvhs_score = analyze(pd.buf, pd.buf_size, TS_DVHS_PACKET_SIZE, 0);
> +        if (dvhs_score > best_score) {
> +            best_score = dvhs_score;
> +            pd_packet_size = TS_DVHS_PACKET_SIZE;
> +        }
> +
> +        fec_score  = analyze(pd.buf, pd.buf_size, TS_FEC_PACKET_SIZE,  0);
> +        if (fec_score > best_score) {
> +            best_score = fec_score;
> +            pd_packet_size = TS_FEC_PACKET_SIZE;
> +        }

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.

> +
> +        av_log(NULL, AV_LOG_TRACE, "Probe size: %d, score: %d, dvhs_score: %d, fec_score: %d \n",
> +            pd.buf_size, score, dvhs_score, fec_score);
> +    }
> +
> +    if (pd.buf)
> +        av_freep(&pd.buf);
> +
> +    return pd_packet_size;
> +
> +fail:
> +    if (pd.buf)
> +        av_freep(&pd.buf);
> +    return AVERROR(ENOMEM);
> }
> 
> typedef struct SectionHeader {
> @@ -2841,8 +2881,6 @@ static int mpegts_read_header(AVFormatContext *s)
> {
>     MpegTSContext *ts = s->priv_data;
>     AVIOContext *pb   = s->pb;
> -    uint8_t buf[8 * 1024] = {0};
> -    int len;
>     int64_t pos, probesize = s->probesize;
>
>     s->internal->prefer_codec_framerate = 1;
> @@ -2850,10 +2888,8 @@ static int mpegts_read_header(AVFormatContext *s)
>     if (ffio_ensure_seekback(pb, probesize) < 0)
>         av_log(s, AV_LOG_WARNING, "Failed to allocate buffers for seekback\n");
> 
> -    /* read the first 8192 bytes to get packet size */
>     pos = avio_tell(pb);
> -    len = avio_read(pb, buf, sizeof(buf));
> -    ts->raw_packet_size = get_packet_size(buf, len);
> +    ts->raw_packet_size = get_packet_size(pb);
>     if (ts->raw_packet_size <= 0) {
>         av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n");
>         ts->raw_packet_size = TS_PACKET_SIZE;
> --

Regards,
Marton
Michael Niedermayer March 6, 2019, 12:25 a.m.
On Mon, Mar 04, 2019 at 10:21:01PM -0500, andriy.gelman@gmail.com wrote:
> From: Andriy Gelman <andriy.gelman@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

[...]
Andriy Gelman March 6, 2019, 3:22 p.m.
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@niedermayer.cc>
wrote:

> On Mon, Mar 04, 2019 at 10:21:01PM -0500, andriy.gelman@gmail.com wrote:
> > From: Andriy Gelman <andriy.gelman@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

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) {
+
+        /*create extra space for next packet*/
+        uint8_t *new_buf = av_realloc(pd.buf, pd.buf_size + PROBE_PACKET_STEP);
+        if (new_buf) {
+            pd.buf = new_buf;
+            ret = avio_read(pb, pd.buf + pd.buf_size, PROBE_PACKET_STEP);
+            if (ret < 0) {
+                av_log(pb, AV_LOG_ERROR, "Error reading from input: %s.\n",
+                   av_err2str(ret));
+                break;
+            }
+            pd.buf_size += ret;
+        } else
+            goto fail;
+
+        /*check score for each fec packet size*/
+        score      = analyze(pd.buf, pd.buf_size, TS_PACKET_SIZE,      0);
+        if (score > best_score) {
+            best_score = score;
+            pd_packet_size = TS_PACKET_SIZE;
+        }
 
-    score      = analyze(buf, size, TS_PACKET_SIZE,      0);
-    dvhs_score = analyze(buf, size, TS_DVHS_PACKET_SIZE, 0);
-    fec_score  = analyze(buf, size, TS_FEC_PACKET_SIZE,  0);
-    av_log(NULL, AV_LOG_TRACE, "score: %d, dvhs_score: %d, fec_score: %d \n",
-            score, dvhs_score, fec_score);
-
-    if (score > fec_score && score > dvhs_score)
-        return TS_PACKET_SIZE;
-    else if (dvhs_score > score && dvhs_score > fec_score)
-        return TS_DVHS_PACKET_SIZE;
-    else if (score < fec_score && dvhs_score < fec_score)
-        return TS_FEC_PACKET_SIZE;
-    else
-        return AVERROR_INVALIDDATA;
+        dvhs_score = analyze(pd.buf, pd.buf_size, TS_DVHS_PACKET_SIZE, 0);
+        if (dvhs_score > best_score) {
+            best_score = dvhs_score;
+            pd_packet_size = TS_DVHS_PACKET_SIZE;
+        }
+
+        fec_score  = analyze(pd.buf, pd.buf_size, TS_FEC_PACKET_SIZE,  0);
+        if (fec_score > best_score) {
+            best_score = fec_score;
+            pd_packet_size = TS_FEC_PACKET_SIZE;
+        }
+
+        av_log(NULL, AV_LOG_TRACE, "Probe size: %d, score: %d, dvhs_score: %d, fec_score: %d \n",
+            pd.buf_size, score, dvhs_score, fec_score);
+    }
+
+    if (pd.buf)
+        av_freep(&pd.buf);
+
+    return pd_packet_size;
+
+fail:
+    if (pd.buf)
+        av_freep(&pd.buf);
+    return AVERROR(ENOMEM);
 }
 
 typedef struct SectionHeader {
@@ -2841,8 +2881,6 @@  static int mpegts_read_header(AVFormatContext *s)
 {
     MpegTSContext *ts = s->priv_data;
     AVIOContext *pb   = s->pb;
-    uint8_t buf[8 * 1024] = {0};
-    int len;
     int64_t pos, probesize = s->probesize;
 
     s->internal->prefer_codec_framerate = 1;
@@ -2850,10 +2888,8 @@  static int mpegts_read_header(AVFormatContext *s)
     if (ffio_ensure_seekback(pb, probesize) < 0)
         av_log(s, AV_LOG_WARNING, "Failed to allocate buffers for seekback\n");
 
-    /* read the first 8192 bytes to get packet size */
     pos = avio_tell(pb);
-    len = avio_read(pb, buf, sizeof(buf));
-    ts->raw_packet_size = get_packet_size(buf, len);
+    ts->raw_packet_size = get_packet_size(pb);
     if (ts->raw_packet_size <= 0) {
         av_log(s, AV_LOG_WARNING, "Could not detect TS packet size, defaulting to non-FEC/DVHS\n");
         ts->raw_packet_size = TS_PACKET_SIZE;