| Message ID | DMr_tEJs796sqRoWzJRXe76wQ1AV8brCbkRjl9sj5HBYlJGLFkyIJQWUVrjZkRwsRrIam31U6IVmD6I5I1tYDrUphEvu6uGEkiolQ3Htl34=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
Andreas, On Mon, 29. Jul 11:11, Andreas Håkon wrote: > Hi, > > An updated version that fixes all previous bugs: > https://patchwork.ffmpeg.org/patch/14109/ > https://patchwork.ffmpeg.org/patch/14099/ > https://patchwork.ffmpeg.org/patch/14036/ > > It passes the tests pointed by Michael Niedermayer (Sample_cut.ts) and > Andriy Gelman (day_flight.mpg). > > I hope this time the patch will be accepted. > Regards. > A.H. > > --- > From 8381febd0e881cfcd53583b0ccdd7eb2c580e422 Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.hakon@protonmail.com> > Date: Mon, 29 Jul 2019 12:02:06 +0100 > Subject: [PATCH] libavformat/mpegtsenc: fix incorrect PCR with multiple > programs [v4] > > The MPEG-TS muxer has a serious bug related to the use of multiple programs: > in that case, the PCR pid selection is incomplete for all services except one. > This patch solves this problem and select correct streams for each program. > Furthermore, it passes all the checks and doesn't generate any regression. > Also fully compatible with shared pids over services. > > You can check it with this example command: > > $ ./ffmpeg -loglevel verbose -y -f data -i /dev/zero \ > -filter_complex "nullsrc=s=60x60,split=2[v0][v1],anullsrc[a]" \ > -map [v0] -c:v:0 rawvideo \ > -map [v1] -c:v:1 rawvideo \ > -map [a] -c:a:0 pcm_s8 \ > -program st=0,2 -program st=1,2 -program st=2 -program st=0 -f mpegts out.ts > > And you will see something like: > > [mpegts @ 0x55f8452797c0] service 1 using PCR in pid=256 > [mpegts @ 0x55f8452797c0] service 2 using PCR in pid=257 > [mpegts @ 0x55f8452797c0] service 3 using PCR in pid=258 > [mpegts @ 0x55f8452797c0] service 4 using PCR in pid=256 > > > Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com> > --- > libavformat/mpegtsenc.c | 167 +++++++++++++++++++++++++++++------------------ > 1 file changed, 105 insertions(+), 62 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index fc0ea22..6a0dd55 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > @@ -60,6 +60,7 @@ typedef struct MpegTSService { > int pcr_packet_count; > int pcr_packet_period; > AVProgram *program; > + AVStream *pcr_st; > } MpegTSService; > > // service_type values as defined in ETSI 300 468 > @@ -774,7 +775,7 @@ static int mpegts_init(AVFormatContext *s) > MpegTSWrite *ts = s->priv_data; > MpegTSWriteStream *ts_st; > MpegTSService *service; > - AVStream *st, *pcr_st = NULL; > + AVStream *st; > AVDictionaryEntry *title, *provider; > int i, j; > const char *service_name; > @@ -831,6 +832,11 @@ static int mpegts_init(AVFormatContext *s) > } > } > > + for (i = 0; i < ts->nb_services; i++) { > + service = ts->services[i]; > + service->pcr_st = NULL; > + } > + If you are going to add pcr_st to MpegTSService then it would make sense to move the initialization to mpegts_add_service function. > ts->pat.pid = PAT_PID; > /* Initialize at 15 so that it wraps and is equal to 0 for the > * first packet we write. */ > @@ -872,17 +878,6 @@ static int mpegts_init(AVFormatContext *s) > goto fail; > } > > - program = av_find_program_from_stream(s, NULL, i); > - if (program) { > - for (j = 0; j < ts->nb_services; j++) { > - if (ts->services[j]->program == program) { > - service = ts->services[j]; > - break; > - } > - } > - } > - > - ts_st->service = service; > /* MPEG pid values < 16 are reserved. Applications which set st->id in > * this range are assigned a calculated pid. */ > if (st->id < 16) { > @@ -895,11 +890,6 @@ static int mpegts_init(AVFormatContext *s) > ret = AVERROR(EINVAL); > goto fail; > } > - if (ts_st->pid == service->pmt.pid) { > - av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); > - ret = AVERROR(EINVAL); > - goto fail; > - } > for (j = 0; j < i; j++) { > if (pids[j] == ts_st->pid) { > av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); > @@ -913,12 +903,7 @@ static int mpegts_init(AVFormatContext *s) > ts_st->first_pts_check = 1; > ts_st->cc = 15; > ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT; > - /* update PCR pid by using the first video stream */ > - if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > - service->pcr_pid == 0x1fff) { > - service->pcr_pid = ts_st->pid; > - pcr_st = st; > - } > + > if (st->codecpar->codec_id == AV_CODEC_ID_AAC && > st->codecpar->extradata_size > 0) { > AVStream *ast; > @@ -949,50 +934,109 @@ static int mpegts_init(AVFormatContext *s) > if (st->codecpar->codec_id == AV_CODEC_ID_OPUS) { > ts_st->opus_pending_trim_start = st->codecpar->initial_padding * 48000 / st->codecpar->sample_rate; > } > + > + /* program service checks */ > + program = av_find_program_from_stream(s, NULL, i); > + do { > + for (j = 0; j < ts->nb_services; j++) { > + if (program) { > + /* search for the services corresponding to this program */ > + if (ts->services[j]->program == program) > + service = ts->services[j]; > + else > + continue; > + } > + > + ts_st->service = service; > + > + /* check for PMT pid conflicts */ > + if (ts_st->pid == service->pmt.pid) { > + av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); > + ret = AVERROR(EINVAL); > + goto fail; > + } > + > + /* update PCR pid by using the first video stream */ > + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > + service->pcr_pid == 0x1fff) { > + service->pcr_pid = ts_st->pid; > + service->pcr_st = st; > + } > + > + /* store this stream as a candidate PCR if the service doesn't have any */ > + if (service->pcr_pid == 0x1fff && > + !service->pcr_st) { > + service->pcr_st = st; > + } > + > + /* exit when just one single service */ > + if (!program) > + break; > + } > + program = program ? av_find_program_from_stream(s, program, i) : NULL; > + } while (program); This may be easier to digest if you separate the cases when s->nb_programs == 0 and s->nb_programs > 0. Maybe something like this could work: /*update service when no programs defined. use default service */ if (s->nb_programs == 0) { ret = update_service_and_set_pcr(s, st, service); if (ret < 0) goto fail: } /*find program for i-th stream */ program = av_find_program_from_stream(s, NULL, i); while (s->nb_programs > 0 && program) { /*find the service that this program belongs to */ for (j = 0; j < ts->nb_services; j++) { if (ts->services[j]->program == program) { service = ts->services[j]; break; } } ret = update_service_and_set_pcr(s, st, service); if (ret < 0) goto fail: /*find next program that this stream belongs to */ program = av_find_program_from_stream(s, program, i); } /*we need to define the function update_service_and_set_pcr */ static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st, MpegTSService *service) { MpegTSWriteStream *ts_st = st->priv_data; ts_st->service = service; /* check for PMT pid conflicts */ if (ts_st->pid == service->pmt.pid) { av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); return AVERROR(EINVAL); } /* update PCR pid by using the first video stream */ if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && service->pcr_pid == 0x1fff) { service->pcr_pid = ts_st->pid; service->pcr_st = st; } /* store this stream as a candidate PCR if the service doesn't have any */ if (service->pcr_pid == 0x1fff) service->pcr_st = st; return 0; } > } > > av_freep(&pids); > > - /* if no video stream, use the first stream as PCR */ > - if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { > - pcr_st = s->streams[0]; > - ts_st = pcr_st->priv_data; > - service->pcr_pid = ts_st->pid; > - } else > - ts_st = pcr_st->priv_data; > - > - if (ts->mux_rate > 1) { > - service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > - (TS_PACKET_SIZE * 8 * 1000); > - > - if (ts->copyts < 1) > - ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > - } else { > - /* Arbitrary values, PAT/PMT will also be written on video key frames */ > - ts->sdt_packet_period = 200; > - ts->pat_packet_period = 40; > - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); > - if (!frame_size) { > - av_log(s, AV_LOG_WARNING, "frame size not set\n"); > - service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * 512); > + for (i = 0; i < ts->nb_services; i++) { > + service = ts->services[i]; > + > + if (!service->pcr_st) { > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\n", service->sid); > + ret = AVERROR(EINVAL); > + goto fail_at_end; fail_at_end is not deallocating any memory. Just return AVERROR(EINVAL) instead of goto would be fine. > + } > + /* get the PCR stream (real or candidate) */ > + ts_st = service->pcr_st->priv_data; > + > + /* if no video stream then use the pid of the candidate PCR */ > + if (service->pcr_pid == 0x1fff) > + service->pcr_pid = ts_st->pid; > + > + if (ts->mux_rate > 1) { > + service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / > + (TS_PACKET_SIZE * 8 * 1000); > + if (ts->copyts < 1) > + ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); > + } else { > + /* Arbitrary values, PAT/PMT will also be written on video key frames */ > + ts->sdt_packet_period = 200; > + ts->pat_packet_period = 40; > + if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { > + int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0); > + if (!frame_size) { > + av_log(s, AV_LOG_WARNING, "frame size not set\n"); > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * 512); > + } else { > + service->pcr_packet_period = > + service->pcr_st->codecpar->sample_rate / (10 * frame_size); > + } > } else { > + // max delta PCR 0.1s > + // TODO: should be avg_frame_rate > service->pcr_packet_period = > - pcr_st->codecpar->sample_rate / (10 * frame_size); > + ts_st->user_tb.den / (10 * ts_st->user_tb.num); > } > - } else { > - // max delta PCR 0.1s > - // TODO: should be avg_frame_rate > - service->pcr_packet_period = > - ts_st->user_tb.den / (10 * ts_st->user_tb.num); > + if (!service->pcr_packet_period) > + service->pcr_packet_period = 1; > } > - if (!service->pcr_packet_period) > - service->pcr_packet_period = 1; > + > + // output a PCR as soon as possible > + service->pcr_packet_count = service->pcr_packet_period; > + > + av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid); > + } > + > + if (!ts_st->service) { > + av_log(s, AV_LOG_ERROR, "empty services!\n"); > + ret = AVERROR(EINVAL); > + goto fail_at_end; > } This is outside the stream loop, so why the random check? > > ts->last_pat_ts = AV_NOPTS_VALUE; > @@ -1005,8 +1049,6 @@ static int mpegts_init(AVFormatContext *s) > ts->sdt_packet_period = INT_MAX; > } > > - // output a PCR as soon as possible > - service->pcr_packet_count = service->pcr_packet_period; > ts->pat_packet_count = ts->pat_packet_period - 1; > ts->sdt_packet_count = ts->sdt_packet_period - 1; > > @@ -1016,7 +1058,7 @@ static int mpegts_init(AVFormatContext *s) > av_log(s, AV_LOG_VERBOSE, "muxrate %d, ", ts->mux_rate); > av_log(s, AV_LOG_VERBOSE, > "pcr every %d pkts, sdt every %d, pat/pmt every %d pkts\n", > - service->pcr_packet_period, > + ts_st->service->pcr_packet_period, > ts->sdt_packet_period, ts->pat_packet_period); > > if (ts->m2ts_mode == -1) { > @@ -1031,6 +1073,7 @@ static int mpegts_init(AVFormatContext *s) > > fail: > av_freep(&pids); > +fail_at_end: > return ret; I don't think you need this goto option. > } > > -- > 1.7.10.4 > Andriy
Hi Andriy, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 1 de August de 2019 0:23, Andriy Gelman <andriy.gelman@gmail.com> wrote: > > + for (i = 0; i < ts->nb_services; i++) { > > + service = ts->services[i]; > > + service->pcr_st = NULL; > > + } > > + > > If you are going to add pcr_st to MpegTSService then it would make sense to move the > initialization to mpegts_add_service function. Good idea! > > + > > + /* program service checks */ > > + program = av_find_program_from_stream(s, NULL, i); > > + do { > > + for (j = 0; j < ts->nb_services; j++) { > > + if (program) { > > + /* search for the services corresponding to this program */ > > + if (ts->services[j]->program == program) > > + service = ts->services[j]; > > + else > > + continue; > > + } > > + > > + ts_st->service = service; > > + > > + /* check for PMT pid conflicts */ > > + if (ts_st->pid == service->pmt.pid) { > > + av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); > > + ret = AVERROR(EINVAL); > > + goto fail; > > + } > > + > > + /* update PCR pid by using the first video stream */ > > + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > > + service->pcr_pid == 0x1fff) { > > + service->pcr_pid = ts_st->pid; > > + service->pcr_st = st; > > + } > > + > > + /* store this stream as a candidate PCR if the service doesn't have any */ > > + if (service->pcr_pid == 0x1fff && > > + !service->pcr_st) { > > + service->pcr_st = st; > > + } > > + > > + /* exit when just one single service */ > > + if (!program) > > + break; > > + } > > + program = program ? av_find_program_from_stream(s, program, i) : NULL; > > + } while (program); > > This may be easier to digest if you separate the cases when > s->nb_programs == 0 and s->nb_programs > 0. Maybe something like this could > work: Well, the code works, right? So let me to comment some things previous to discuss your suggestion: 1. I prefer to leave the code close to the original one. If someone needs to do more changes, it's preferable to do small changes. Don't you think so? 2. The difference between "s->nb_programs == 0" and "s->nb_programs > 0" is because the old code. From my point of view, the special case when "s->nb_programs == 0" is a little sloppy. However, I need to consider it, so for this reason the code handles it. > /*update service when no programs defined. use default service */ > if (s->nb_programs == 0) { > > ret = update_service_and_set_pcr(s, st, service); > if (ret < 0) > goto fail: > > > } > > /*find program for i-th stream */ > program = av_find_program_from_stream(s, NULL, i); > while (s->nb_programs > 0 && program) { > > /*find the service that this program belongs to */ > for (j = 0; j < ts->nb_services; j++) { > > if (ts->services[j]->program == program) { > > service = ts->services[j]; > > break; > } > } > No, no! The loop requires to complete it for all services! Every service requires a PCR, and the PCR can be inside a shared stream. So if we break for the first service, then other services will not be processed. Please, see that the code is inside a loop and uses a continue when the program doesn't match. > > ret = update_service_and_set_pcr(s, st, service); > if (ret < 0) > goto fail: > > /*find next program that this stream belongs to */ > program = av_find_program_from_stream(s, program, i); > } > > /*we need to define the function update_service_and_set_pcr */ > static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st, MpegTSService *service) > { > > MpegTSWriteStream *ts_st = st->priv_data; > > ts_st->service = service; > > > /* check for PMT pid conflicts */ > if (ts_st->pid == service->pmt.pid) { > > av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\\n", ts_st->pid); > > return AVERROR(EINVAL); > } > > > /* update PCR pid by using the first video stream */ > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > > service->pcr_pid == 0x1fff) { > > service->pcr_pid = ts_st->pid; > > service->pcr_st = st; > > } > > > /* store this stream as a candidate PCR if the service doesn't have any */ > if (service->pcr_pid == 0x1fff) > > service->pcr_st = st; > > > return 0; > } As commented before IMHO the code is more clear if we leave the service checks here and not in a new function. When others update the code will see that "all" checks requires to be completed here. > + for (i = 0; i < ts->nb_services; i++) { > + service = ts->services[i]; > + > + if (!service->pcr_st) { > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\n", service->sid); > + ret = AVERROR(EINVAL); > + goto fail_at_end; > > fail_at_end is not deallocating any memory. > Just return AVERROR(EINVAL) instead of goto would be fine. Well, in the future perhaps some other change will require to do more processing at end. So, I prefer to exit using the same behaviour as the rest of the code. It's good coding practice, so I won't change it. > + if (!ts_st->service) { > + av_log(s, AV_LOG_ERROR, "empty services!\n"); > + ret = AVERROR(EINVAL); > + goto fail_at_end; > } > > This is outside the stream loop, so why the random check? This block is like an ASSERT. The code NEVER needs to continue from this point if no service is created for the Transport Stream. So it checks it. It's an insurance policy for future changes. > > > > fail: > > av_freep(&pids); > > > +fail_at_end: > > return ret; > > I don't think you need this goto option. As I've already said, I'd rather leave it this way. Regards. A.H. ---
Andreas, On Thu, 01. Aug 08:24, Andreas Håkon wrote: > Hi Andriy, > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Thursday, 1 de August de 2019 0:23, Andriy Gelman <andriy.gelman@gmail.com> wrote: > > > > + for (i = 0; i < ts->nb_services; i++) { > > > + service = ts->services[i]; > > > + service->pcr_st = NULL; > > > + } > > > + > > > > If you are going to add pcr_st to MpegTSService then it would make sense to move the > > initialization to mpegts_add_service function. > > Good idea! > > > > > + > > > + /* program service checks */ > > > + program = av_find_program_from_stream(s, NULL, i); > > > + do { > > > + for (j = 0; j < ts->nb_services; j++) { > > > + if (program) { > > > + /* search for the services corresponding to this program */ > > > + if (ts->services[j]->program == program) > > > + service = ts->services[j]; > > > + else > > > + continue; > > > + } > > > + > > > + ts_st->service = service; > > > + > > > + /* check for PMT pid conflicts */ > > > + if (ts_st->pid == service->pmt.pid) { > > > + av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); > > > + ret = AVERROR(EINVAL); > > > + goto fail; > > > + } > > > + > > > + /* update PCR pid by using the first video stream */ > > > + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > > > + service->pcr_pid == 0x1fff) { > > > + service->pcr_pid = ts_st->pid; > > > + service->pcr_st = st; > > > + } > > > + > > > + /* store this stream as a candidate PCR if the service doesn't have any */ > > > + if (service->pcr_pid == 0x1fff && > > > + !service->pcr_st) { > > > + service->pcr_st = st; > > > + } > > > + > > > + /* exit when just one single service */ > > > + if (!program) > > > + break; > > > + } > > > + program = program ? av_find_program_from_stream(s, program, i) : NULL; > > > + } while (program); > > > > This may be easier to digest if you separate the cases when > > s->nb_programs == 0 and s->nb_programs > 0. Maybe something like this could > > work: > > Well, the code works, right? > So let me to comment some things previous to discuss your suggestion: > > 1. I prefer to leave the code close to the original one. If someone needs to do > more changes, it's preferable to do small changes. Don't you think so? > > 2. The difference between "s->nb_programs == 0" and "s->nb_programs > 0" is because > the old code. From my point of view, the special case when "s->nb_programs == 0" > is a little sloppy. However, I need to consider it, so for this reason the code > handles it. Yes, I believe the code works. But, readability is also very important. Think about someone who is not familiar with the code. It's tough to see straightaway how the two cases s->nb_programs == 0 and and s->nb_programs > 0 are handled differently. Also, I believe it's easier for bugs to be appear if someone modifies this code. I think Marton or Michael should have the final say. > > > /*update service when no programs defined. use default service */ > > if (s->nb_programs == 0) { > > > > ret = update_service_and_set_pcr(s, st, service); > > if (ret < 0) > > goto fail: > > > > > > } > > > > /*find program for i-th stream */ > > program = av_find_program_from_stream(s, NULL, i); > > while (s->nb_programs > 0 && program) { > > > > /*find the service that this program belongs to */ > > for (j = 0; j < ts->nb_services; j++) { > > > > if (ts->services[j]->program == program) { > > > > service = ts->services[j]; > > > > break; > > } > > } > > > > No, no! The loop requires to complete it for all services! > Every service requires a PCR, and the PCR can be inside a > shared stream. So if we break for the first service, then > other services will not be processed. > Please, see that the code is inside a loop and uses a continue > when the program doesn't match. The break refers to the "for" loop and not the "while" loop. You'll find the next service in the next iteration of the while loop. > > > > > ret = update_service_and_set_pcr(s, st, service); > > if (ret < 0) > > goto fail: > > > > /*find next program that this stream belongs to */ > > program = av_find_program_from_stream(s, program, i); > > } > > > > /*we need to define the function update_service_and_set_pcr */ > > static int update_service_and_set_pcr(AVFormatContext *s, AVStream *st, MpegTSService *service) > > { > > > > MpegTSWriteStream *ts_st = st->priv_data; > > > > ts_st->service = service; > > > > > > /* check for PMT pid conflicts */ > > if (ts_st->pid == service->pmt.pid) { > > > > av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\\n", ts_st->pid); > > > > return AVERROR(EINVAL); > > } > > > > > > /* update PCR pid by using the first video stream */ > > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && > > > > service->pcr_pid == 0x1fff) { > > > > service->pcr_pid = ts_st->pid; > > > > service->pcr_st = st; > > > > } > > > > > > /* store this stream as a candidate PCR if the service doesn't have any */ > > if (service->pcr_pid == 0x1fff) > > > > service->pcr_st = st; > > > > > > return 0; > > } > > As commented before IMHO the code is more clear if we leave the service > checks here and not in a new function. When others update the code will > see that "all" checks requires to be completed here. I suppose the checks could be outside the function. > > > > + for (i = 0; i < ts->nb_services; i++) { > > + service = ts->services[i]; > > + > > + if (!service->pcr_st) { > > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\n", service->sid); > > + ret = AVERROR(EINVAL); > > + goto fail_at_end; > > > > fail_at_end is not deallocating any memory. > > Just return AVERROR(EINVAL) instead of goto would be fine. > > Well, in the future perhaps some other change will require to do more processing > at end. So, I prefer to exit using the same behaviour as the rest of the code. > It's good coding practice, so I won't change it. I did a grep for goto in ffmpeg. I cannot see any cases where there is a branch to simply return the error. We should probably be consistent with the rest of the code. > > > > + if (!ts_st->service) { > > + av_log(s, AV_LOG_ERROR, "empty services!\n"); > > + ret = AVERROR(EINVAL); > > + goto fail_at_end; > > } > > > > This is outside the stream loop, so why the random check? > > This block is like an ASSERT. The code NEVER needs to continue from this > point if no service is created for the Transport Stream. So it checks it. > It's an insurance policy for future changes. The check is outside of the services loop. ts_st can belong to any of the streams. Did you mean for the check to be inside the loop? If it's an assert, then an appropriate assert function would be better? > > > > > > > > fail: > > > av_freep(&pids); > > > > > +fail_at_end: > > > return ret; > > > > I don't think you need this goto option. > > As I've already said, I'd rather leave it this way. > > > Regards. > A.H. > > --- > Andriy
Hi Andriy, ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, 1 de August de 2019 16:15, Andriy Gelman <andriy.gelman@gmail.com> wrote: > > > If you are going to add pcr_st to MpegTSService then it would make sense to move the > > > initialization to mpegts_add_service function. > > > > Good idea! Marton recommends eliminating the unnecessary initialization. > > > > > > This may be easier to digest if you separate the cases when > > > s->nb_programs == 0 and s->nb_programs > 0. Maybe something like this could > > > work: > > > > Well, the code works, right? > > So let me to comment some things previous to discuss your suggestion: > > > > 1. I prefer to leave the code close to the original one. If someone needs to do > > more changes, it's preferable to do small changes. Don't you think so? > > > > 2. The difference between "s->nb_programs == 0" and "s->nb_programs > 0" is because > > the old code. From my point of view, the special case when "s->nb_programs == 0" > > is a little sloppy. However, I need to consider it, so for this reason the code > > handles it. > > > > Yes, I believe the code works. Thank you! > But, readability is also very important. Think about someone who is not familiar > with the code. It's tough to see straightaway how the two cases s->nb_programs == 0 and > and s->nb_programs > 0 are handled differently. Also, I believe it's easier for bugs to be > appear if someone modifies this code. I think so too! But, the cases are (in reverse order): A) One stream in multiple services. B) One stream in one service. C) No programs, so just one service. Because (A) and (B) we need to iterate over all services. Futhermore, for each stream we need to search for a program associated to it. But, an extreme case still remains: (C); when no program is defined. And only for this special case can the code be somewhat confusing. I will try to add some comments. > I think Marton or Michael should have the final say. Sure! And the comments from Marton are nice. > > > /*update service when no programs defined. use default service */ > > > if (s->nb_programs == 0) { > > > > > > ret = update_service_and_set_pcr(s, st, service); > > > if (ret < 0) > > > goto fail: > > > > > > > > > } > > > /*find program for i-th stream */ > > > program = av_find_program_from_stream(s, NULL, i); > > > while (s->nb_programs > 0 && program) { > > > /*find the service that this program belongs to */ > > > for (j = 0; j < ts->nb_services; j++) { > > > > > > if (ts->services[j]->program == program) { > > > > > > service = ts->services[j]; > > > > > > break; > > > } > > > } > > > > > > > No, no! The loop requires to complete it for all services! > > Every service requires a PCR, and the PCR can be inside a > > shared stream. So if we break for the first service, then > > other services will not be processed. > > Please, see that the code is inside a loop and uses a continue > > when the program doesn't match. > > The break refers to the "for" loop and not the "while" loop. > You'll find the next service in the next iteration of the while loop. Sorry? In the "next" iteration of the while loop the "j" will return to start at 0, so you don't iterate over ALL services in where this stream lives. The "j" iterator requires to go over all services, and execute the inner block for all cases. That's the core of this patch to solve the current bug! > > > > As commented before IMHO the code is more clear if we leave the service > > checks here and not in a new function. When others update the code will > > see that "all" checks requires to be completed here. > > I suppose the checks could be outside the function. OK. If you read the entire source file after applying the patch, you can see that several checks have been made for the stream. I make some rearrangements to separate PID number checks from SERVICE checks. So instead of a separated function called from the outher loop of the function "mpegts_init()" I prefer this approach of inline code. > > > + for (i = 0; i < ts->nb_services; i++) { > > > + service = ts->services[i]; > > > + > > > + if (!service->pcr_st) { > > > + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\\n", service->sid); > > > + ret = AVERROR(EINVAL); > > > + goto fail_at_end; > > > > > > fail_at_end is not deallocating any memory. > > > Just return AVERROR(EINVAL) instead of goto would be fine. > > > > Well, in the future perhaps some other change will require to do more processing > > at end. So, I prefer to exit using the same behaviour as the rest of the code. > > It's good coding practice, so I won't change it. > > I did a grep for goto in ffmpeg. I cannot see any cases where there is a branch to > simply return the error. We should probably be consistent with the rest of the > code. OK. > > > + if (!ts_st->service) { > > > + av_log(s, AV_LOG_ERROR, "empty services!\\n"); > > > + ret = AVERROR(EINVAL); > > > + goto fail_at_end; > > > } > > > > > > > > > This is outside the stream loop, so why the random check? > > > > This block is like an ASSERT. The code NEVER needs to continue from this > > point if no service is created for the Transport Stream. So it checks it. > > It's an insurance policy for future changes. > > The check is outside of the services loop. ts_st can belong to any of the > streams. Did you mean for the check to be inside the loop? > > If it's an assert, then an appropriate assert function would be better? Marton suggested that. I'll do it that way. > Andriy Thank you for your comments! Regards. A.H. ---
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index fc0ea22..6a0dd55 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -60,6 +60,7 @@ typedef struct MpegTSService { int pcr_packet_count; int pcr_packet_period; AVProgram *program; + AVStream *pcr_st; } MpegTSService; // service_type values as defined in ETSI 300 468 @@ -774,7 +775,7 @@ static int mpegts_init(AVFormatContext *s) MpegTSWrite *ts = s->priv_data; MpegTSWriteStream *ts_st; MpegTSService *service; - AVStream *st, *pcr_st = NULL; + AVStream *st; AVDictionaryEntry *title, *provider; int i, j; const char *service_name; @@ -831,6 +832,11 @@ static int mpegts_init(AVFormatContext *s) } } + for (i = 0; i < ts->nb_services; i++) { + service = ts->services[i]; + service->pcr_st = NULL; + } + ts->pat.pid = PAT_PID; /* Initialize at 15 so that it wraps and is equal to 0 for the * first packet we write. */ @@ -872,17 +878,6 @@ static int mpegts_init(AVFormatContext *s) goto fail; } - program = av_find_program_from_stream(s, NULL, i); - if (program) { - for (j = 0; j < ts->nb_services; j++) { - if (ts->services[j]->program == program) { - service = ts->services[j]; - break; - } - } - } - - ts_st->service = service; /* MPEG pid values < 16 are reserved. Applications which set st->id in * this range are assigned a calculated pid. */ if (st->id < 16) { @@ -895,11 +890,6 @@ static int mpegts_init(AVFormatContext *s) ret = AVERROR(EINVAL); goto fail; } - if (ts_st->pid == service->pmt.pid) { - av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); - ret = AVERROR(EINVAL); - goto fail; - } for (j = 0; j < i; j++) { if (pids[j] == ts_st->pid) { av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); @@ -913,12 +903,7 @@ static int mpegts_init(AVFormatContext *s) ts_st->first_pts_check = 1; ts_st->cc = 15; ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT; - /* update PCR pid by using the first video stream */ - if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && - service->pcr_pid == 0x1fff) { - service->pcr_pid = ts_st->pid; - pcr_st = st; - } + if (st->codecpar->codec_id == AV_CODEC_ID_AAC && st->codecpar->extradata_size > 0) { AVStream *ast; @@ -949,50 +934,109 @@ static int mpegts_init(AVFormatContext *s) if (st->codecpar->codec_id == AV_CODEC_ID_OPUS) { ts_st->opus_pending_trim_start = st->codecpar->initial_padding * 48000 / st->codecpar->sample_rate; } + + /* program service checks */ + program = av_find_program_from_stream(s, NULL, i); + do { + for (j = 0; j < ts->nb_services; j++) { + if (program) { + /* search for the services corresponding to this program */ + if (ts->services[j]->program == program) + service = ts->services[j]; + else + continue; + } + + ts_st->service = service; + + /* check for PMT pid conflicts */ + if (ts_st->pid == service->pmt.pid) { + av_log(s, AV_LOG_ERROR, "Duplicate stream id %d\n", ts_st->pid); + ret = AVERROR(EINVAL); + goto fail; + } + + /* update PCR pid by using the first video stream */ + if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && + service->pcr_pid == 0x1fff) { + service->pcr_pid = ts_st->pid; + service->pcr_st = st; + } + + /* store this stream as a candidate PCR if the service doesn't have any */ + if (service->pcr_pid == 0x1fff && + !service->pcr_st) { + service->pcr_st = st; + } + + /* exit when just one single service */ + if (!program) + break; + } + program = program ? av_find_program_from_stream(s, program, i) : NULL; + } while (program); } av_freep(&pids); - /* if no video stream, use the first stream as PCR */ - if (service->pcr_pid == 0x1fff && s->nb_streams > 0) { - pcr_st = s->streams[0]; - ts_st = pcr_st->priv_data; - service->pcr_pid = ts_st->pid; - } else - ts_st = pcr_st->priv_data; - - if (ts->mux_rate > 1) { - service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / - (TS_PACKET_SIZE * 8 * 1000); - ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / - (TS_PACKET_SIZE * 8 * 1000); - ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / - (TS_PACKET_SIZE * 8 * 1000); - - if (ts->copyts < 1) - ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); - } else { - /* Arbitrary values, PAT/PMT will also be written on video key frames */ - ts->sdt_packet_period = 200; - ts->pat_packet_period = 40; - if (pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { - int frame_size = av_get_audio_frame_duration2(pcr_st->codecpar, 0); - if (!frame_size) { - av_log(s, AV_LOG_WARNING, "frame size not set\n"); - service->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * 512); + for (i = 0; i < ts->nb_services; i++) { + service = ts->services[i]; + + if (!service->pcr_st) { + av_log(s, AV_LOG_ERROR, "no PCR selected for the service %i\n", service->sid); + ret = AVERROR(EINVAL); + goto fail_at_end; + } + /* get the PCR stream (real or candidate) */ + ts_st = service->pcr_st->priv_data; + + /* if no video stream then use the pid of the candidate PCR */ + if (service->pcr_pid == 0x1fff) + service->pcr_pid = ts_st->pid; + + if (ts->mux_rate > 1) { + service->pcr_packet_period = (int64_t)ts->mux_rate * ts->pcr_period / + (TS_PACKET_SIZE * 8 * 1000); + ts->sdt_packet_period = (int64_t)ts->mux_rate * SDT_RETRANS_TIME / + (TS_PACKET_SIZE * 8 * 1000); + ts->pat_packet_period = (int64_t)ts->mux_rate * PAT_RETRANS_TIME / + (TS_PACKET_SIZE * 8 * 1000); + if (ts->copyts < 1) + ts->first_pcr = av_rescale(s->max_delay, PCR_TIME_BASE, AV_TIME_BASE); + } else { + /* Arbitrary values, PAT/PMT will also be written on video key frames */ + ts->sdt_packet_period = 200; + ts->pat_packet_period = 40; + if (service->pcr_st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) { + int frame_size = av_get_audio_frame_duration2(service->pcr_st->codecpar, 0); + if (!frame_size) { + av_log(s, AV_LOG_WARNING, "frame size not set\n"); + service->pcr_packet_period = + service->pcr_st->codecpar->sample_rate / (10 * 512); + } else { + service->pcr_packet_period = + service->pcr_st->codecpar->sample_rate / (10 * frame_size); + } } else { + // max delta PCR 0.1s + // TODO: should be avg_frame_rate service->pcr_packet_period = - pcr_st->codecpar->sample_rate / (10 * frame_size); + ts_st->user_tb.den / (10 * ts_st->user_tb.num); } - } else { - // max delta PCR 0.1s - // TODO: should be avg_frame_rate - service->pcr_packet_period = - ts_st->user_tb.den / (10 * ts_st->user_tb.num); + if (!service->pcr_packet_period) + service->pcr_packet_period = 1; } - if (!service->pcr_packet_period) - service->pcr_packet_period = 1; + + // output a PCR as soon as possible + service->pcr_packet_count = service->pcr_packet_period; + + av_log(s, AV_LOG_VERBOSE, "service %i using PCR in pid=%i\n", service->sid, service->pcr_pid); + } + + if (!ts_st->service) { + av_log(s, AV_LOG_ERROR, "empty services!\n"); + ret = AVERROR(EINVAL); + goto fail_at_end; } ts->last_pat_ts = AV_NOPTS_VALUE; @@ -1005,8 +1049,6 @@ static int mpegts_init(AVFormatContext *s) ts->sdt_packet_period = INT_MAX; } - // output a PCR as soon as possible - service->pcr_packet_count = service->pcr_packet_period; ts->pat_packet_count = ts->pat_packet_period - 1; ts->sdt_packet_count = ts->sdt_packet_period - 1; @@ -1016,7 +1058,7 @@ static int mpegts_init(AVFormatContext *s) av_log(s, AV_LOG_VERBOSE, "muxrate %d, ", ts->mux_rate); av_log(s, AV_LOG_VERBOSE, "pcr every %d pkts, sdt every %d, pat/pmt every %d pkts\n", - service->pcr_packet_period, + ts_st->service->pcr_packet_period, ts->sdt_packet_period, ts->pat_packet_period); if (ts->m2ts_mode == -1) { @@ -1031,6 +1073,7 @@ static int mpegts_init(AVFormatContext *s) fail: av_freep(&pids); +fail_at_end: return ret; }