diff mbox series

[FFmpeg-devel,1/2] avformat/mpegtsenc: Don't use heap allocated array to store pids

Message ID 20200430195728.32671-1-andriy.gelman@gmail.com
State Accepted
Commit 1dd0def976405c38b80e89a385e3eaad98988358
Headers show
Series [FFmpeg-devel,1/2] avformat/mpegtsenc: Don't use heap allocated array to store pids | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andriy Gelman April 30, 2020, 7:57 p.m. UTC 
From: Andriy Gelman <andriy.gelman@gmail.com>

A temporary heap array currently stores pids from all streams.  It is
used to make sure there are no duplicated pids. However, this array is
not needed because the pids from past streams are stored in the
MpegTSWriteStream structs.

Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
---
 libavformat/mpegtsenc.c | 51 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

Comments

Marton Balint May 1, 2020, 3:54 p.m. UTC | #1 
On Thu, 30 Apr 2020, Andriy Gelman wrote:

> From: Andriy Gelman <andriy.gelman@gmail.com>
>
> A temporary heap array currently stores pids from all streams.  It is
> used to make sure there are no duplicated pids. However, this array is
> not needed because the pids from past streams are stored in the
> MpegTSWriteStream structs.
>

LGTM, thanks.

Marton

> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> ---
> libavformat/mpegtsenc.c | 51 +++++++++++------------------------------
> 1 file changed, 14 insertions(+), 37 deletions(-)
>
> diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
> index f2be6c6632..8ca1ddf003 100644
> --- a/libavformat/mpegtsenc.c
> +++ b/libavformat/mpegtsenc.c
> @@ -931,7 +931,6 @@ static int mpegts_init(AVFormatContext *s)
> {
>     MpegTSWrite *ts = s->priv_data;
>     int i, j;
> -    int *pids;
>     int ret;
>
>     if (ts->m2ts_mode == -1) {
> @@ -989,12 +988,6 @@ static int mpegts_init(AVFormatContext *s)
>     ts->sdt.write_packet = section_write_packet;
>     ts->sdt.opaque       = s;
> 
> -    pids = av_malloc_array(s->nb_streams, sizeof(*pids));
> -    if (!pids) {
> -        ret = AVERROR(ENOMEM);
> -        goto fail;
> -    }
> -
>     /* assign pids to each stream */
>     for (i = 0; i < s->nb_streams; i++) {
>         AVStream *st = s->streams[i];
> @@ -1002,8 +995,7 @@ static int mpegts_init(AVFormatContext *s)
>
>         ts_st = av_mallocz(sizeof(MpegTSWriteStream));
>         if (!ts_st) {
> -            ret = AVERROR(ENOMEM);
> -            goto fail;
> +            return AVERROR(ENOMEM);
>         }
>         st->priv_data = ts_st;
> 
> @@ -1011,8 +1003,7 @@ static int mpegts_init(AVFormatContext *s)
>
>         ts_st->payload = av_mallocz(ts->pes_payload_size);
>         if (!ts_st->payload) {
> -            ret = AVERROR(ENOMEM);
> -            goto fail;
> +            return AVERROR(ENOMEM);
>         }
>
>         /* MPEG pid values < 16 are reserved. Applications which set st->id in
> @@ -1043,8 +1034,7 @@ static int mpegts_init(AVFormatContext *s)
>                     ts->m2ts_textsub_pid > M2TS_TEXTSUB_PID + 1        ||
>                     ts_st->pid < 16) {
>                     av_log(s, AV_LOG_ERROR, "Cannot automatically assign PID for stream %d\n", st->index);
> -                    ret = AVERROR(EINVAL);
> -                    goto fail;
> +                    return AVERROR(EINVAL);
>                 }
>             } else {
>                 ts_st->pid = ts->start_pid + i;
> @@ -1055,30 +1045,26 @@ static int mpegts_init(AVFormatContext *s)
>         if (ts_st->pid >= 0x1FFF) {
>             av_log(s, AV_LOG_ERROR,
>                    "Invalid stream id %d, must be less than 8191\n", st->id);
> -            ret = AVERROR(EINVAL);
> -            goto fail;
> +            return AVERROR(EINVAL);
>         }
>         for (j = 0; j < ts->nb_services; j++) {
>             if (ts->services[j]->pmt.pid > LAST_OTHER_PID) {
>                 av_log(s, AV_LOG_ERROR,
>                        "Invalid PMT PID %d, must be less than %d\n", ts->services[j]->pmt.pid, LAST_OTHER_PID + 1);
> -                ret = AVERROR(EINVAL);
> -                goto fail;
> +                return AVERROR(EINVAL);
>             }
>             if (ts_st->pid == ts->services[j]->pmt.pid) {
>                 av_log(s, AV_LOG_ERROR, "PID %d cannot be both elementary and PMT PID\n", ts_st->pid);
> -                ret = AVERROR(EINVAL);
> -                goto fail;
> +                return AVERROR(EINVAL);
>             }
>         }
>         for (j = 0; j < i; j++) {
> -            if (pids[j] == ts_st->pid) {
> +            MpegTSWriteStream *ts_st_prev = s->streams[j]->priv_data;
> +            if (ts_st_prev->pid == ts_st->pid) {
>                 av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid);
> -                ret = AVERROR(EINVAL);
> -                goto fail;
> +                return AVERROR(EINVAL);
>             }
>         }
> -        pids[i]                = ts_st->pid;
>         ts_st->payload_pts     = AV_NOPTS_VALUE;
>         ts_st->payload_dts     = AV_NOPTS_VALUE;
>         ts_st->first_pts_check = 1;
> @@ -1089,35 +1075,30 @@ static int mpegts_init(AVFormatContext *s)
>             AVStream *ast;
>             ts_st->amux = avformat_alloc_context();
>             if (!ts_st->amux) {
> -                ret = AVERROR(ENOMEM);
> -                goto fail;
> +                return AVERROR(ENOMEM);
>             }
>             ts_st->amux->oformat =
>                 av_guess_format((ts->flags & MPEGTS_FLAG_AAC_LATM) ? "latm" : "adts",
>                                 NULL, NULL);
>             if (!ts_st->amux->oformat) {
> -                ret = AVERROR(EINVAL);
> -                goto fail;
> +                return AVERROR(EINVAL);
>             }
>             if (!(ast = avformat_new_stream(ts_st->amux, NULL))) {
> -                ret = AVERROR(ENOMEM);
> -                goto fail;
> +                return AVERROR(ENOMEM);
>             }
>             ret = avcodec_parameters_copy(ast->codecpar, st->codecpar);
>             if (ret != 0)
> -                goto fail;
> +                return ret;
>             ast->time_base = st->time_base;
>             ret = avformat_write_header(ts_st->amux, NULL);
>             if (ret < 0)
> -                goto fail;
> +                return ret;
>         }
>         if (st->codecpar->codec_id == AV_CODEC_ID_OPUS) {
>             ts_st->opus_pending_trim_start = st->codecpar->initial_padding * 48000 / st->codecpar->sample_rate;
>         }
>     }
> 
> -    av_freep(&pids);
> -
>     if (ts->copyts < 1)
>         ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
> 
> @@ -1138,10 +1119,6 @@ static int mpegts_init(AVFormatContext *s)
>            av_rescale(ts->pat_period, 1000, PCR_TIME_BASE));
>
>     return 0;
> -
> -fail:
> -    av_freep(&pids);
> -    return ret;
> }
> 
> /* send SDT, PAT and PMT tables regularly */
> -- 
> 2.25.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andriy Gelman May 12, 2020, 3:11 a.m. UTC | #2 
On Fri, 01. May 17:54, Marton Balint wrote:
> 
> 
> On Thu, 30 Apr 2020, Andriy Gelman wrote:
> 
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > A temporary heap array currently stores pids from all streams.  It is
> > used to make sure there are no duplicated pids. However, this array is
> > not needed because the pids from past streams are stored in the
> > MpegTSWriteStream structs.
> > 
> 
> LGTM, thanks.
> 
> Marton
> 

Hi Marton, 

Thanks for the review.
Is it ok to push both patches?
Marton Balint May 12, 2020, 8:37 a.m. UTC | #3 
On Mon, 11 May 2020, Andriy Gelman wrote:

> On Fri, 01. May 17:54, Marton Balint wrote:
>> 
>> 
>> On Thu, 30 Apr 2020, Andriy Gelman wrote:
>> 
>> > From: Andriy Gelman <andriy.gelman@gmail.com>
>> > 
>> > A temporary heap array currently stores pids from all streams.  It is
>> > used to make sure there are no duplicated pids. However, this array is
>> > not needed because the pids from past streams are stored in the
>> > MpegTSWriteStream structs.
>> > 
>> 
>> LGTM, thanks.
>> 
>> Marton
>> 
>
> Hi Marton, 
>
> Thanks for the review.
> Is it ok to push both patches?

Sure, go ahead.

Thanks,
Marton
Andriy Gelman May 14, 2020, 2:32 p.m. UTC | #4 
On Tue, 12. May 10:37, Marton Balint wrote:
> 
> 
> On Mon, 11 May 2020, Andriy Gelman wrote:
> 
> > On Fri, 01. May 17:54, Marton Balint wrote:
> > > 
> > > 
> > > On Thu, 30 Apr 2020, Andriy Gelman wrote:
> > > 
> > > > From: Andriy Gelman <andriy.gelman@gmail.com>
> > > > > A temporary heap array currently stores pids from all streams.
> > > It is
> > > > used to make sure there are no duplicated pids. However, this array is
> > > > not needed because the pids from past streams are stored in the
> > > > MpegTSWriteStream structs.
> > > >
> > > 
> > > LGTM, thanks.
> > > 
> > > Marton
> > > 
> > 
> > Hi Marton,
> > 
> > Thanks for the review.
> > Is it ok to push both patches?

> 
> Sure, go ahead.
> 

Applied

Thanks,
diff mbox series

Patch

diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index f2be6c6632..8ca1ddf003 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -931,7 +931,6 @@  static int mpegts_init(AVFormatContext *s)
 {
     MpegTSWrite *ts = s->priv_data;
     int i, j;
-    int *pids;
     int ret;
 
     if (ts->m2ts_mode == -1) {
@@ -989,12 +988,6 @@  static int mpegts_init(AVFormatContext *s)
     ts->sdt.write_packet = section_write_packet;
     ts->sdt.opaque       = s;
 
-    pids = av_malloc_array(s->nb_streams, sizeof(*pids));
-    if (!pids) {
-        ret = AVERROR(ENOMEM);
-        goto fail;
-    }
-
     /* assign pids to each stream */
     for (i = 0; i < s->nb_streams; i++) {
         AVStream *st = s->streams[i];
@@ -1002,8 +995,7 @@  static int mpegts_init(AVFormatContext *s)
 
         ts_st = av_mallocz(sizeof(MpegTSWriteStream));
         if (!ts_st) {
-            ret = AVERROR(ENOMEM);
-            goto fail;
+            return AVERROR(ENOMEM);
         }
         st->priv_data = ts_st;
 
@@ -1011,8 +1003,7 @@  static int mpegts_init(AVFormatContext *s)
 
         ts_st->payload = av_mallocz(ts->pes_payload_size);
         if (!ts_st->payload) {
-            ret = AVERROR(ENOMEM);
-            goto fail;
+            return AVERROR(ENOMEM);
         }
 
         /* MPEG pid values < 16 are reserved. Applications which set st->id in
@@ -1043,8 +1034,7 @@  static int mpegts_init(AVFormatContext *s)
                     ts->m2ts_textsub_pid > M2TS_TEXTSUB_PID + 1        ||
                     ts_st->pid < 16) {
                     av_log(s, AV_LOG_ERROR, "Cannot automatically assign PID for stream %d\n", st->index);
-                    ret = AVERROR(EINVAL);
-                    goto fail;
+                    return AVERROR(EINVAL);
                 }
             } else {
                 ts_st->pid = ts->start_pid + i;
@@ -1055,30 +1045,26 @@  static int mpegts_init(AVFormatContext *s)
         if (ts_st->pid >= 0x1FFF) {
             av_log(s, AV_LOG_ERROR,
                    "Invalid stream id %d, must be less than 8191\n", st->id);
-            ret = AVERROR(EINVAL);
-            goto fail;
+            return AVERROR(EINVAL);
         }
         for (j = 0; j < ts->nb_services; j++) {
             if (ts->services[j]->pmt.pid > LAST_OTHER_PID) {
                 av_log(s, AV_LOG_ERROR,
                        "Invalid PMT PID %d, must be less than %d\n", ts->services[j]->pmt.pid, LAST_OTHER_PID + 1);
-                ret = AVERROR(EINVAL);
-                goto fail;
+                return AVERROR(EINVAL);
             }
             if (ts_st->pid == ts->services[j]->pmt.pid) {
                 av_log(s, AV_LOG_ERROR, "PID %d cannot be both elementary and PMT PID\n", ts_st->pid);
-                ret = AVERROR(EINVAL);
-                goto fail;
+                return AVERROR(EINVAL);
             }
         }
         for (j = 0; j < i; j++) {
-            if (pids[j] == ts_st->pid) {
+            MpegTSWriteStream *ts_st_prev = s->streams[j]->priv_data;
+            if (ts_st_prev->pid == ts_st->pid) {
                 av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid);
-                ret = AVERROR(EINVAL);
-                goto fail;
+                return AVERROR(EINVAL);
             }
         }
-        pids[i]                = ts_st->pid;
         ts_st->payload_pts     = AV_NOPTS_VALUE;
         ts_st->payload_dts     = AV_NOPTS_VALUE;
         ts_st->first_pts_check = 1;
@@ -1089,35 +1075,30 @@  static int mpegts_init(AVFormatContext *s)
             AVStream *ast;
             ts_st->amux = avformat_alloc_context();
             if (!ts_st->amux) {
-                ret = AVERROR(ENOMEM);
-                goto fail;
+                return AVERROR(ENOMEM);
             }
             ts_st->amux->oformat =
                 av_guess_format((ts->flags & MPEGTS_FLAG_AAC_LATM) ? "latm" : "adts",
                                 NULL, NULL);
             if (!ts_st->amux->oformat) {
-                ret = AVERROR(EINVAL);
-                goto fail;
+                return AVERROR(EINVAL);
             }
             if (!(ast = avformat_new_stream(ts_st->amux, NULL))) {
-                ret = AVERROR(ENOMEM);
-                goto fail;
+                return AVERROR(ENOMEM);
             }
             ret = avcodec_parameters_copy(ast->codecpar, st->codecpar);
             if (ret != 0)
-                goto fail;
+                return ret;
             ast->time_base = st->time_base;
             ret = avformat_write_header(ts_st->amux, NULL);
             if (ret < 0)
-                goto fail;
+                return ret;
         }
         if (st->codecpar->codec_id == AV_CODEC_ID_OPUS) {
             ts_st->opus_pending_trim_start = st->codecpar->initial_padding * 48000 / st->codecpar->sample_rate;
         }
     }
 
-    av_freep(&pids);
-
     if (ts->copyts < 1)
         ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE);
 
@@ -1138,10 +1119,6 @@  static int mpegts_init(AVFormatContext *s)
            av_rescale(ts->pat_period, 1000, PCR_TIME_BASE));
 
     return 0;
-
-fail:
-    av_freep(&pids);
-    return ret;
 }
 
 /* send SDT, PAT and PMT tables regularly */