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 |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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".
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?
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
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 --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 */