| Message ID | BVbVGqjNLz7nhZD_VPMQRfuJ2EcACFthBGsvAY_2P1fuEx4y1y02BEr3ODpdAix2nX-dAM1sR7ePn_4xZpy5PCz_kkFmPmMeO70lACS-CoI=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
Hi, An additional comment on the matter... ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Monday, 10 de June de 2019 19:29, Andreas Håkon <andreas.hakon@protonmail.com> wrote: > Hi, > > --- > > --- > From aa02575cc11bed0fd2ae2a01368c8673ad48e64b Mon Sep 17 00:00:00 2001 > > From: Andreas Hakon < > andreas.hakon@protonmail.com >> > > Date: Mon, 10 Jun 2019 18:14:56 +0100 > Subject: [PATCH] libavformat/mpegtsenc: new interlaced mux mode > > This patch implements a new optional "parallel muxing mode" in the MPEGTS muxer. > The strategy that implements the current mux (selected by default) is based on > writing full PES packages sequentially. This mode can be problematic when using > with DTV broadcasts, as some large video PES packets can delay the writing of > other elementary streams. > The new optional parameter "-mpegts_extra_mux 1" enables a different strategy. > Instead of writing all PES packets sequentially, the first TS packet of each PES > packet is written when the PES packet is started. But the rest of the PES data > will be written later, and interlaced between all the mux streams. > This new (optional) behavior has clear advantages when multiplexing multiple > programs with several video streams. And although this does not turn the > current implementation into a professional muxer, it brings the result closer > to what professional equipment does. > > Signed-off-by: Andreas Hakon < > andreas.hakon@protonmail.com >> > > --- > libavformat/mpegtsenc.c | 305 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 233 insertions(+), 72 deletions(-) This patch line's old! +/* NOTE: The return value is negative when the onlyone flag generates the exit. */ Please, remove it. Other than this, the rest of the patch is valid. We've tested it for weeks and see no issues. Regards. A.H. ---
Hello, On Mon, 10. Jun 17:29, Andreas Håkon wrote: > Hi, > > Here is a list of comments on this patch: > (Note: I use for all the tests the file https://samples.ffmpeg.org/HDTV/bshi01.tp) > > - By default the current behavior is selected. You can verify that this > patch doesn’t alter the original behavior with this simple test: > > $ ffmpeg-original -i bshi01.tp \ > -c:v copy -c:a copy -c:d copy \ > -f mpegts -muxrate 22M bshi01-stock.ts > > $ ffmpeg-patched -i bshi01.tp \ > -c:v copy -c:a copy -c:d copy \ > -f mpegts -muxrate 22M -mpegts_extra_mux 0 bshi01- new.ts > > $ cmp -b bshi01-stock.ts bshi01-new.ts > > So both files are identical. The patch therefore doesn’t introduce any > changes in the implementation of the sequential mode. > > - To check the new interlaced mode you can perform this other test: > > $ ffmpeg-patched -y -loglevel verbose -i bshi01.tp \ > -map "i:0x100" -c:0 copy \ > -map "i:0x110" -c:a:0 mp2 -ac:0 2 -ar:0 48000 -ab:0 384k \ > -map "i:0x130" -c:2 copy \ > -map "i:0x110" -c:3 copy \ > -map "i:0x100" -c:4 copy \ > -program title=Prog1:st=0:st=1:st=2 \ > -program title=Prog2:st=3:st=4 \ > -f mpegts -muxrate 44M -mpegts_extra_mux 1 bshi01-mode1.ts > > $ ffmpeg-patched -y -loglevel verbose -i bshi01.tp \ > -map "i:0x100" -c:0 copy \ > -map "i:0x110" -c:a:0 mp2 -ac:0 2 -ar:0 48000 -ab:0 384k \ > -map "i:0x130" -c:2 copy \ > -map "i:0x110" -c:3 copy \ > -map "i:0x100" -c:4 copy \ > -program title=Prog1:st=0:st=1:st=2 \ > -program title=Prog2:st=3:st=4 \ > -f mpegts -muxrate 44M -mpegts_extra_mux 0 bshi01-mode0.ts > > And you can observe: > > a) The size of the files “bshi01-mode0.ts” and “bshi01-mode1.ts” is > almost the same. If you inspect the content, you can verify that the > difference is based solely on: a) an small increase in the number of > NULL packets in mode 1; b) a few new packets with only PCR and > not payload in the first video stream. > > b) If you demux the three files to elemental streams, then you can > check that the content is identical. Using the linux package “tstools” > you can do this check: > > $ ts2es -pid 256 bshi01-mode0.ts bshi01-mode0-256.m2v > $ ts2es -pid 260 bshi01-mode0.ts bshi01-mode0-260.m2v > $ ts2es -pid 257 bshi01-mode0.ts bshi01-mode0-257.mp2 > $ ts2es -pid 259 bshi01-mode0.ts bshi01-mode0-259.aac > > $ ts2es -pid 256 bshi01-mode1.ts bshi01-mode1-256.m2v > $ ts2es -pid 260 bshi01-mode1.ts bshi01-mode1-260.m2v > $ ts2es -pid 257 bshi01-mode1.ts bshi01-mode1-257.mp2 > $ ts2es -pid 259 bshi01-mode1.ts bshi01-mode1-259.aac > > c) If you look at the internal content of the files you can verify that > the original “bshi01.tp” file has all pids interlaced, but this isn’t true > for the file “bshi01-mode0.ts”. However, the file “bshi01-mode1.ts” > has an internal structure similar to that of the original file. > You can view the content using the well-known tool > “DVB Inspector” with the “Grid View” option. > > These tests confirm the correctness of the implementation of this > new multiplexing mode. > > - Last but not least, this patch fixes a bug within the current > implementation. When several programs are used in the same > TS file, the function “mpegts_init()” incorrectly initializes the > “service->pcr_pid” when several services are used. The current > code only sets this value for the last service, and leaves the > others uninitialized. This patch solves the problem by looping over > all services to establish the correct values. > > Please check this patch so that it can be accepted. > Regards. > A.H. > > --- > From aa02575cc11bed0fd2ae2a01368c8673ad48e64b Mon Sep 17 00:00:00 2001 > From: Andreas Hakon <andreas.hakon@protonmail.com> > Date: Mon, 10 Jun 2019 18:14:56 +0100 > Subject: [PATCH] libavformat/mpegtsenc: new interlaced mux mode > > This patch implements a new optional "parallel muxing mode" in the MPEGTS muxer. > The strategy that implements the current mux (selected by default) is based on > writing full PES packages sequentially. This mode can be problematic when using > with DTV broadcasts, as some large video PES packets can delay the writing of > other elementary streams. Could you go into more detail as to why this causes problems for DTV broadcasts? > The new optional parameter "-mpegts_extra_mux 1" enables a different strategy. > Instead of writing all PES packets sequentially, the first TS packet of each PES > packet is written when the PES packet is started. But the rest of the PES data > will be written later, and interlaced between all the mux streams. > This new (optional) behavior has clear advantages when multiplexing multiple > programs with several video streams. And although this does not turn the > current implementation into a professional muxer, it brings the result closer > to what professional equipment does. > > Signed-off-by: Andreas Hakon <andreas.hakon@protonmail.com> > --- > libavformat/mpegtsenc.c | 305 ++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 233 insertions(+), 72 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index fc0ea22..93f8d3d 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > @@ -97,6 +97,7 @@ typedef struct MpegTSWrite { > int pmt_start_pid; > int start_pid; > int m2ts_mode; > + int parallel_mux; > > int reemit_pat_pmt; // backward compatibility > > @@ -120,6 +121,7 @@ typedef struct MpegTSWrite { > /* a PES packet header is generated every DEFAULT_PES_HEADER_FREQ packets */ > #define DEFAULT_PES_HEADER_FREQ 16 > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) > +#define MAX_PES_PAYLOAD 2 * 200 * 1024 // From mpegts.c > > /* The section length is 12 bits. The first 2 are set to 0, the remaining > * 10 bits should not exceed 1021. */ > @@ -227,17 +229,28 @@ static int mpegts_write_section1(MpegTSSection *s, int tid, int id, > #define PAT_RETRANS_TIME 100 > #define PCR_RETRANS_TIME 20 > > +#define PES_START 1 /* 0000 0001 */ > +#define PES_FULL 2 /* 0000 0010 */ > +#define UNUSED_FLAG_3 4 /* 0000 0100 */ > +#define UNUSED_FLAG_4 8 /* 0000 1000 */ > +#define UNUSED_FLAG_5 16 /* 0001 0000 */ > +#define UNUSED_FLAG_6 32 /* 0010 0000 */ > +#define UNUSED_FLAG_7 64 /* 0100 0000 */ Is it relevant to include these? > +#define PES_NEEDS_END 128 /* 1000 0000 */ > + > typedef struct MpegTSWriteStream { > struct MpegTSService *service; > int pid; /* stream associated pid */ > int cc; > int discontinuity; > int payload_size; > + int payload_top; > int first_pts_check; ///< first pts check needed > int prev_payload_key; > int64_t payload_pts; > int64_t payload_dts; > int payload_flags; > + int pes_flags; > uint8_t *payload; > AVFormatContext *amux; > AVRational user_tb; > @@ -866,7 +879,7 @@ static int mpegts_init(AVFormatContext *s) > ts_st->user_tb = st->time_base; > avpriv_set_pts_info(st, 33, 1, 90000); > > - ts_st->payload = av_mallocz(ts->pes_payload_size); > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); Could you clarify why this needs to be changed? > if (!ts_st->payload) { > ret = AVERROR(ENOMEM); > goto fail; > @@ -910,6 +923,8 @@ static int mpegts_init(AVFormatContext *s) > pids[i] = ts_st->pid; > ts_st->payload_pts = AV_NOPTS_VALUE; > ts_st->payload_dts = AV_NOPTS_VALUE; > + ts_st->payload_top = 0; > + ts_st->pes_flags = 0; > ts_st->first_pts_check = 1; > ts_st->cc = 15; > ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT; > @@ -953,46 +968,52 @@ static int mpegts_init(AVFormatContext *s) > > 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 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); > + } else { > + service->pcr_packet_period = > + 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; > } Why do you need the loop over the services here? It seems unrelated unless I missed something. > > ts->last_pat_ts = AV_NOPTS_VALUE; > @@ -1005,8 +1026,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; > > @@ -1172,9 +1191,14 @@ static uint8_t *get_ts_payload_start(uint8_t *pkt) > /* Add a PES header to the front of the payload, and segment into an integer > * number of TS packets. The final TS packet is padded using an oversized > * adaptation header to exactly fill the last TS packet. > - * NOTE: 'payload' contains a complete PES payload. */ > -static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > - const uint8_t *payload, int payload_size, > + * NOTE: 'payload' contains a complete PES payload. > + * NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); > + * When 'mode' = -1 it writes the entire payload with the header; > + * when 'mode' = 0 it writes only one TS packet with the header; > + * when 'mode' > 0 it writes only one TS packet. Enum for mode would make more sense to me > + * Note3: It returns the number of writed bytes. */ > +static int mpegts_write_pes(AVFormatContext *s, AVStream *st, > + const uint8_t *payload, int payload_size, int mode, > int64_t pts, int64_t dts, int key, int stream_id) > { > MpegTSWriteStream *ts_st = st->priv_data; > @@ -1186,13 +1210,14 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > int64_t pcr = -1; /* avoid warning */ > int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE); > int force_pat = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && key && !ts_st->prev_payload_key; > + int ret_size = 0; > > av_assert0(ts_st->payload != buf || st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO); > if (ts->flags & MPEGTS_FLAG_PAT_PMT_AT_FRAMES && st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { > force_pat = 1; > } > > - is_start = 1; > + is_start = (mode > 0 || mode < -1) ? 0 : 1; > while (payload_size > 0) { > retransmit_si_info(s, force_pat, dts); > force_pat = 0; > @@ -1389,6 +1414,8 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > memset(q, 0xff, pes_header_stuffing_bytes); > q += pes_header_stuffing_bytes; > } > + if (is_dvb_subtitle) > + ts_st->pes_flags |= PES_NEEDS_END; > is_start = 0; > } > /* header size */ > @@ -1420,7 +1447,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > } > } > > - if (is_dvb_subtitle && payload_size == len) { > + if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) { This seems unrelated to your commit.. If it is, you can remove PES_NEEDS_END > memcpy(buf + TS_PACKET_SIZE - len, payload, len - 1); > buf[TS_PACKET_SIZE - 1] = 0xff; /* end_of_PES_data_field_marker: an 8-bit field with fixed contents 0xff for DVB subtitle */ > } else { > @@ -1431,8 +1458,12 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > payload_size -= len; > mpegts_prefix_m2ts_header(s); > avio_write(s->pb, buf, TS_PACKET_SIZE); > + ret_size += len; > + if (mode >= 0) > + payload_size = 0; // Write one packet only then exit Why not just break? > } > ts_st->prev_payload_key = key; > + return ret_size; > } > > int ff_check_h264_startcode(AVFormatContext *s, const AVStream *st, const AVPacket *pkt) > @@ -1519,6 +1550,102 @@ static int opus_get_packet_samples(AVFormatContext *s, AVPacket *pkt) > return duration; > } > > +static inline int write_full_pes_stream(AVFormatContext *s, AVStream *st, > + const uint8_t *payload, int payload_size, > + int64_t pts, int64_t dts, int key, int stream_id) > +{ > + return mpegts_write_pes(s, st, payload, payload_size, -1, pts, dts, key, stream_id); > +} > + > +static inline int write_flush_pes_stream(AVFormatContext *s, AVStream *st, > + const uint8_t *payload, int payload_size, > + int64_t pts, int64_t dts, int key, int stream_id) > +{ > + return mpegts_write_pes(s, st, payload, payload_size, -2, pts, dts, key, stream_id); > +} > + > +static inline int write_pkt_pes_stream(AVFormatContext *s, AVStream *st, > + const uint8_t *payload, int payload_size, int mode, > + int64_t pts, int64_t dts, int key, int stream_id) > +{ > + return mpegts_write_pes(s, st, payload, payload_size, mode, pts, dts, key, stream_id); > +} > + > +static int write_side_streams(AVFormatContext *s, int64_t dts, int64_t delay, int stream_id, int parallel) > +{ > + int i; > + int sum = 0; > + int check_mode; > + int write_mode; // -1: Don't write anything > + // 0: Start the PES packet and write 1 TS packet > + // 1: Continue writing just 1 TS packet > + // 2: Write full PES packet > + int ret; > + for(i=0; i<s->nb_streams; i++) { > + AVStream *st2 = s->streams[i]; > + MpegTSWriteStream *ts_st2 = st2->priv_data; > + > + check_mode = parallel ? (ts_st2->payload_top > 0) : 0; > + > + if (ts_st2->payload_size && !check_mode > + && (ts_st2->payload_dts == AV_NOPTS_VALUE || dts - ts_st2->payload_dts > delay/2 || (ts_st2->pes_flags & PES_START))) { > + write_mode = parallel ? 0 : 2; > + } else if (check_mode) { > + write_mode = 1; > + } else { > + write_mode = -1; > + } > + > + switch (write_mode) > + { > + case 2: // SEQUENTIAL MODE > + ret = write_full_pes_stream(s, st2, ts_st2->payload, ts_st2->payload_size, > + ts_st2->payload_pts, ts_st2->payload_dts, > + ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); > + ts_st2->payload_size = 0; > + sum += ret; > + break; > + case 1: // PARALLEL MODE > + case 0: > + ret = write_pkt_pes_stream(s, st2, ts_st2->payload + ts_st2->payload_top, > + ts_st2->payload_size - ts_st2->payload_top, write_mode, > + ts_st2->payload_pts, ts_st2->payload_dts, > + ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); > + ts_st2->payload_top += ret; > + sum += ret; > + break; > + default: > + continue; > + } > + > + if (ts_st2->payload_size && ts_st2->payload_top == ts_st2->payload_size) { > + ts_st2->payload_size = 0; > + ts_st2->payload_top = 0; > + ts_st2->pes_flags = 0; > + } > + } > + return sum; > +} > + > +/* NOTE: The return value is negative when the onlyone flag generates the exit. */ > +static int write_with_side_streams(AVFormatContext *s, AVStream *st, int payload_top, > + const uint8_t *payload, int payload_size, > + int64_t pts, int64_t dts, int64_t delay, int key, int stream_id) > +{ > + int force = payload_size - payload_top > MAX_PES_PAYLOAD ? 1 : 0; > + if (force) > + av_log(s, AV_LOG_WARNING, "PES packet oversized, full sequential writing required\n"); > + if (payload_size && payload_top != payload_size) { > + do { > + payload_top += mpegts_write_pes(s, st, payload + payload_top, > + payload_size - payload_top, payload_top ? 1 : 0, > + pts, dts, key, stream_id); > + } while (force && payload_size != payload_top); > + } > + write_side_streams(s, dts, delay, stream_id, 1); > + return payload_top; > +} > + > static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > { > AVStream *st = s->streams[pkt->stream_index]; > @@ -1739,53 +1866,77 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > } > } > > - if (pkt->dts != AV_NOPTS_VALUE) { > - int i; > - for(i=0; i<s->nb_streams; i++) { > - AVStream *st2 = s->streams[i]; > - MpegTSWriteStream *ts_st2 = st2->priv_data; > - if ( ts_st2->payload_size > - && (ts_st2->payload_dts == AV_NOPTS_VALUE || dts - ts_st2->payload_dts > delay/2)) { > - mpegts_write_pes(s, st2, ts_st2->payload, ts_st2->payload_size, > - ts_st2->payload_pts, ts_st2->payload_dts, > - ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); > - ts_st2->payload_size = 0; > - } > - } > + // Write pending PES packets (SEQUENTIAL MODE) > + if (pkt->dts != AV_NOPTS_VALUE && !ts->parallel_mux) { > + write_side_streams(s, dts, delay, stream_id, 0); > } > > + // Complete a new PES packet (new incoming data will go into another PES) > if (ts_st->payload_size && (ts_st->payload_size + size > ts->pes_payload_size || > - (dts != AV_NOPTS_VALUE && ts_st->payload_dts != AV_NOPTS_VALUE && > - av_compare_ts(dts - ts_st->payload_dts, st->time_base, > - s->max_delay, AV_TIME_BASE_Q) >= 0) || > - ts_st->opus_queued_samples + opus_samples >= 5760 /* 120ms */)) { > - mpegts_write_pes(s, st, ts_st->payload, ts_st->payload_size, > - ts_st->payload_pts, ts_st->payload_dts, > - ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); > - ts_st->payload_size = 0; > - ts_st->opus_queued_samples = 0; > + (dts != AV_NOPTS_VALUE && ts_st->payload_dts != AV_NOPTS_VALUE && > + av_compare_ts(dts - ts_st->payload_dts, st->time_base, > + s->max_delay, AV_TIME_BASE_Q) >= 0) || > + ts_st->opus_queued_samples + opus_samples >= 5760 /* 120ms */)) { > + if (!ts->parallel_mux || ts_st->opus_queued_samples) { > + write_full_pes_stream(s, st, ts_st->payload, ts_st->payload_size, > + ts_st->payload_pts, ts_st->payload_dts, > + ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); > + ts_st->payload_size = 0; > + ts_st->opus_queued_samples = 0; > + } else { > + ts_st->pes_flags |= PES_START | PES_FULL; > + } > + } > + > + // Write pending PES packets (PARALLEL MODE) > + if (pkt->dts != AV_NOPTS_VALUE && ts->parallel_mux && (ts_st->pes_flags & PES_START)) { > + // Empty previous pending PES packet before starting a new one or write one packet > + do { > + write_with_side_streams(s, st, 0, > + ts_st->payload, 0, > + ts_st->payload_pts, ts_st->payload_dts, delay, > + ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); > + } while (ts_st->pes_flags & PES_START); > } > > + // Directly write a new PES packet with the incoming data > if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO || size > ts->pes_payload_size) { > av_assert0(!ts_st->payload_size); > // for video and subtitle, write a single pes packet > - mpegts_write_pes(s, st, buf, size, pts, dts, > - pkt->flags & AV_PKT_FLAG_KEY, stream_id); > - ts_st->opus_queued_samples = 0; > - av_free(data); > - return 0; > + if (!ts->parallel_mux || ts_st->opus_queued_samples) { > + write_full_pes_stream(s, st, buf, size, pts, dts, > + pkt->flags & AV_PKT_FLAG_KEY, stream_id); > + ts_st->opus_queued_samples = 0; > + goto free; > + } else { > + ts_st->payload_top = write_with_side_streams(s, st, 0, > + buf, size, > + pts, dts, delay, > + pkt->flags & AV_PKT_FLAG_KEY, stream_id); > + if (ts_st->payload_top == size) { > + ts_st->payload_size = 0; > + ts_st->payload_top = 0; > + ts_st->pes_flags = 0; > + goto free; > + } > + ts_st->pes_flags |= PES_START; > + ts_st->payload_size = 0; // this value will be set later > + } > } > > + // Start a new PES packet > if (!ts_st->payload_size) { > ts_st->payload_pts = pts; > ts_st->payload_dts = dts; > ts_st->payload_flags = pkt->flags; > } > > + // Enqueue data in the current PES packet > memcpy(ts_st->payload + ts_st->payload_size, buf, size); > ts_st->payload_size += size; > ts_st->opus_queued_samples += opus_samples; > > +free: > av_free(data); > > return 0; > @@ -1794,13 +1945,20 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > static void mpegts_write_flush(AVFormatContext *s) > { > int i; > + MpegTSWrite *ts = s->priv_data; > > /* flush current packets */ > for (i = 0; i < s->nb_streams; i++) { > AVStream *st = s->streams[i]; > MpegTSWriteStream *ts_st = st->priv_data; > if (ts_st->payload_size > 0) { > - mpegts_write_pes(s, st, ts_st->payload, ts_st->payload_size, > + if (!ts->parallel_mux || ts_st->payload_top == 0) > + write_full_pes_stream(s, st, ts_st->payload, ts_st->payload_size, > + ts_st->payload_pts, ts_st->payload_dts, > + ts_st->payload_flags & AV_PKT_FLAG_KEY, -1); > + else > + write_flush_pes_stream(s, st, ts_st->payload + ts_st->payload_top, > + ts_st->payload_size - ts_st->payload_top, > ts_st->payload_pts, ts_st->payload_dts, > ts_st->payload_flags & AV_PKT_FLAG_KEY, -1); > ts_st->payload_size = 0; > @@ -1920,6 +2078,9 @@ static const AVOption options[] = { > { "mpegts_m2ts_mode", "Enable m2ts mode.", > offsetof(MpegTSWrite, m2ts_mode), AV_OPT_TYPE_BOOL, > { .i64 = -1 }, -1, 1, AV_OPT_FLAG_ENCODING_PARAM }, > + { "mpegts_extra_mux", "Enable Non-Sequential muxing mode.", > + offsetof(MpegTSWrite, parallel_mux), AV_OPT_TYPE_BOOL, > + { .i64 = 0 }, 0, 1, AV_OPT_FLAG_ENCODING_PARAM }, > { "muxrate", NULL, > offsetof(MpegTSWrite, mux_rate), AV_OPT_TYPE_INT, > { .i64 = 1 }, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM }, > -- > 1.7.10.4 > > _______________________________________________ > 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
Hi Andriy, I'm glad you're interested in this patch. > > This patch implements a new optional "parallel muxing mode" in the MPEGTS muxer. > > The strategy that implements the current mux (selected by default) is based on > > writing full PES packages sequentially. This mode can be problematic when using > > with DTV broadcasts, as some large video PES packets can delay the writing of > > other elementary streams. > > Could you go into more detail as to why this causes problems for DTV broadcasts? Because two reasons: 1) When substreams of multiple services are interlaced, broadcast errors are minimized. Burst errors in the signal only interfere with a small number (or just one) packet of each program, so it's possible that the error is camouflaged. In contrast, when using CONSECUTIVE pid packets, burst errors are more significant. 2) Due to buffer restrictions on DTV broadcasts. If you include more than one video stream with large PES packets, sequential writing can cause buffer problems at reception because other PES packets arrive a little later. > > +#define PES_START 1 /* 0000 0001 / > > +#define PES_FULL 2 / 0000 0010 */ > > > +#define UNUSED_FLAG_3 4 /* 0000 0100 / > > +#define UNUSED_FLAG_4 8 / 0000 1000 / > > +#define UNUSED_FLAG_5 16 / 0001 0000 / > > +#define UNUSED_FLAG_6 32 / 0010 0000 / > > +#define UNUSED_FLAG_7 64 / 0100 0000 */ > > Is it relevant to include these? No. It's not necessary. However, they make the code more compressible and can be used in the future for other flags. > > - ts_st->payload = av_mallocz(ts->pes_payload_size); > > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); > > Could you clarify why this needs to be changed? Sure! Because when you write in parallel you're delaying the writing of the PES packet. So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file: #define DEFAULT_PES_HEADER_FREQ 16 #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) > > + for (i = 0; i < ts->nb_services; i++) { > > + service = ts->services[i]; > > ... > > } > > Why do you need the loop over the services here? It seems unrelated unless > I missed something. I explained it in my original message... The current code has a bug and it doesn't set correctly the value of the service->pcr_pid. And this patch requires correct values of pcr_pid for each program. Please note that this patch makes complete sense when mixing multiple video streams, such as when using multiple programs. > > - - NOTE: 'payload' contains a complete PES payload. > > - - NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); > > - - When 'mode' = -1 it writes the entire payload with the header; > > - - when 'mode' = 0 it writes only one TS packet with the header; > > - - when 'mode' > 0 it writes only one TS packet. > > > > Enum for mode would make more sense to me Yes and not. The code is more compact in this case with numerical values, as you don't need to do checks when you call to the function. > > + if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) { > > This seems unrelated to your commit.. > If it is, you can remove PES_NEEDS_END Sorry, no! That's completely related to this patch. Let me explain: Writing Telext PES packets it's an special case. The function writes at end 1 byte. The flag PES_NEEDS_END is used in this case to indicate this case. Please note that the function with this patch is called multiple times for the same PES packet. And only when writing the header at the start this check can be determined. So we need to save this information. If not, some packets are malformed. > > + payload_size = 0; // Write one packet only then exit > > Why not just break? Sorry? That's the last line of the loop! What's the advantage of breaking the loop here? I would understand the meaning of a return, but we need to execute the last part of the code before the end return. So a return is out of scope. We'd better make the code clear and simple. > > -- > > Andriy More questions? A.H. ---
On Thu, 13 Jun 2019, Andreas Håkon wrote: > Hi Andriy, > > I'm glad you're interested in this patch. > > > >> > This patch implements a new optional "parallel muxing mode" in the MPEGTS muxer. >> > The strategy that implements the current mux (selected by default) is based on >> > writing full PES packages sequentially. This mode can be problematic when using >> > with DTV broadcasts, as some large video PES packets can delay the writing of >> > other elementary streams. >> >> Could you go into more detail as to why this causes problems for DTV broadcasts? > > Because two reasons: > > 1) When substreams of multiple services are interlaced, broadcast errors are minimized. > Burst errors in the signal only interfere with a small number (or just one) packet > of each program, so it's possible that the error is camouflaged. > In contrast, when using CONSECUTIVE pid packets, burst errors are more significant. > > 2) Due to buffer restrictions on DTV broadcasts. If you include more than one video > stream with large PES packets, sequential writing can cause buffer problems at > reception because other PES packets arrive a little later. > Ok. But I believe the correct term for this is "interleaved". I have never heard "interlaced" used for TS packets. > > >> > +#define PES_START 1 /* 0000 0001 / >> > +#define PES_FULL 2 / 0000 0010 */ >> >> > +#define UNUSED_FLAG_3 4 /* 0000 0100 / >> > +#define UNUSED_FLAG_4 8 / 0000 1000 / >> > +#define UNUSED_FLAG_5 16 / 0001 0000 / >> > +#define UNUSED_FLAG_6 32 / 0010 0000 / >> > +#define UNUSED_FLAG_7 64 / 0100 0000 */ >> >> Is it relevant to include these? > > No. It's not necessary. However, they make the code more compressible and can be > used in the future for other flags. Don't include them if they are not needed. It would be also a good idea to name them xxx_FLAG if these are indeed flags. e.g.: PES_START_FLAG PES_FULL_FLAG PES_NEEDS_END_FLAG also I see no reason that PES_NEEDS_END is 128. Just make it 4. And keep the indentation pretty. > > >> > - ts_st->payload = av_mallocz(ts->pes_payload_size); >> > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); >> >> Could you clarify why this needs to be changed? > > Sure! Because when you write in parallel you're delaying the writing of the PES packet. > So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to > a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file: > > #define DEFAULT_PES_HEADER_FREQ 16 > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) OK, but you shold override ts->pes_payload_size, and not the malloc, no? There is code in mpegts_init which sets pes_payload_size, you should force it to MAX_PES_PAYLOAD there before doing the round up. > > > >> > + for (i = 0; i < ts->nb_services; i++) { >> > + service = ts->services[i]; >> > ... >> > } >> >> Why do you need the loop over the services here? It seems unrelated unless >> I missed something. > > I explained it in my original message... > The current code has a bug and it doesn't set correctly the value of the > service->pcr_pid. And this patch requires correct values of pcr_pid for each > program. Please note that this patch makes complete sense when mixing multiple > video streams, such as when using multiple programs. This should be a separate patch then. I suggest implementing this before adding interleaving support. > > > >> > - - NOTE: 'payload' contains a complete PES payload. >> > - - NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); >> > - - When 'mode' = -1 it writes the entire payload with the header; >> > - - when 'mode' = 0 it writes only one TS packet with the header; >> > - - when 'mode' > 0 it writes only one TS packet. >> > >> >> Enum for mode would make more sense to me > > Yes and not. The code is more compact in this case with numerical values, as you > don't need to do checks when you call to the function. The code might be more compact, and tons of less readable. Use enums, or defines. You might also use flags for mode, if that makes the code more readable. > > > >> > + if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) { >> >> This seems unrelated to your commit.. >> If it is, you can remove PES_NEEDS_END > > Sorry, no! That's completely related to this patch. Let me explain: > > Writing Telext PES packets it's an special case. The function writes at end 1 > byte. The flag PES_NEEDS_END is used in this case to indicate this case. Please > note that the function with this patch is called multiple times for the same PES > packet. And only when writing the header at the start this check can be determined. > So we need to save this information. If not, some packets are malformed. > > > >> > + payload_size = 0; // Write one packet only then exit >> >> Why not just break? > > Sorry? That's the last line of the loop! What's the advantage of breaking the loop here? Because you break out of it later anyway since the loop condition is "payload_size > 0". Regards, Marton
Hi Marton, > Ok. But I believe the correct term for this is "interleaved". I have > never heard "interlaced" used for TS packets. I feel you're right: the name "interleaved mux mode" is preferable. Thank you for pointing it out! > > > > +#define PES_START 1 /* 0000 0001 / > > > > +#define PES_FULL 2 / 0000 0010 */ > > > > > > > +#define UNUSED_FLAG_3 4 /* 0000 0100 / > > > > +#define UNUSED_FLAG_4 8 / 0000 1000 / > > > > +#define UNUSED_FLAG_5 16 / 0001 0000 / > > > > +#define UNUSED_FLAG_6 32 / 0010 0000 / > > > > +#define UNUSED_FLAG_7 64 / 0100 0000 */ > > > > > > Is it relevant to include these? > > > > No. It's not necessary. However, they make the code more compressible and can be > > used in the future for other flags. > > Don't include them if they are not needed. It would be also a good idea > to name them xxx_FLAG if these are indeed flags. e.g.: > > PES_START_FLAG > PES_FULL_FLAG > PES_NEEDS_END_FLAG OK. I agree. > also I see no reason that PES_NEEDS_END is 128. Just make it 4. Ummm... The reason for leaving values unused is for future improvements. If another developer needs to use a new FLAG, it is mandatory to avoid collisions. Perhaps a simple comment like that is preferable: /* Unused free flags: 4,8,16,32,64 */ > And keep the indentation pretty. In the patch the identation is correct! > > > > - ts_st->payload = av_mallocz(ts->pes_payload_size); > > > > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); > > > > > > Could you clarify why this needs to be changed? > > > > Sure! Because when you write in parallel you're delaying the writing of the PES packet. > > So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to > > a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file: > > #define DEFAULT_PES_HEADER_FREQ 16 > > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) > > OK, but you shold override ts->pes_payload_size, and not the malloc, no? > There is code in mpegts_init which sets pes_payload_size, you should force > it to MAX_PES_PAYLOAD there before doing the round up. No, no! If I override the ts->pes_payload_size then when not using the interleaved some unused memory will be allocated. Please note that when using the sequential mode the payload is writed "directly" to the output stream. And this is true with PES packets with a size greater than pes_payload_size (i.e. video). So the ts->pes_payload_size is used only to store small writes until a PES packet is complete. However, when using the interleaved mode the entire PES packet requires to be saved until the last part is writed in the output stream. For this reason the malloc has different values. > > > > + for (i = 0; i < ts->nb_services; i++) { > > > > + service = ts->services[i]; > > > > ... > > > > } > > > > > > > > > > Why do you need the loop over the services here? It seems unrelated unless > > > I missed something. > > > > I explained it in my original message... > > The current code has a bug and it doesn't set correctly the value of the > > service->pcr_pid. And this patch requires correct values of pcr_pid for each > > program. Please note that this patch makes complete sense when mixing multiple > > video streams, such as when using multiple programs. > > This should be a separate patch then. I suggest implementing this before > adding interleaving support. Yes and not... 1. This interleaved mode requires correct value of the service->pcr_pid, as it have sense with multiple programs. So the "patch" here is mandatory. We can assume that it's part of the new mux mode. 2. With the sequential mode, the service->pcr_pid bug is hidden, because no one uses multiple programs. So a lot of work for me for an irrelevant fix. So I'd prefer to just comment here, and make it all one single change. > > > > + - NOTE: 'payload' contains a complete PES payload. > > > > + - NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); > > > > + - When 'mode' = -1 it writes the entire payload with the header; > > > > + - when 'mode' = 0 it writes only one TS packet with the header; > > > > + - when 'mode' > 0 it writes only one TS packet. > > > > > > > > > > Enum for mode would make more sense to me > > > > Yes and not. The code is more compact in this case with numerical values, as you > > don't need to do checks when you call to the function. > > The code might be more compact, and tons of less readable. Use enums, or > defines. You might also use flags for mode, if that makes the code more > readable. I'm sorry, no. In my opinion it's more legible and compact like that. I accept all your comments, and I appreciate them. But in this case I do not see the advantage. > > > > + payload_size = 0; // Write one packet only then exit > > > > > > > > > > Why not just break? > > > > Sorry? That's the last line of the loop! What's the advantage of breaking the loop here? > > Because you break out of it later anyway since the loop condition is > "payload_size > 0". OK. With a "break;" instead of "payload_size = 0;" it will be the same. Thank you for reviewing! Regards. A.H. ---
Hi,
> Thank you for reviewing!
New version [v2] at: https://patchwork.ffmpeg.org/patch/13745/
Regards.
A.H.
---
On Wed, 26 Jun 2019, Andreas Håkon wrote: >> > > > - ts_st->payload = av_mallocz(ts->pes_payload_size); >> > > > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); >> > > >> > > Could you clarify why this needs to be changed? >> > >> > Sure! Because when you write in parallel you're delaying the writing of the PES packet. >> > So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to >> > a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file: >> > #define DEFAULT_PES_HEADER_FREQ 16 >> > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) >> >> OK, but you shold override ts->pes_payload_size, and not the malloc, no? >> There is code in mpegts_init which sets pes_payload_size, you should force >> it to MAX_PES_PAYLOAD there before doing the round up. > > No, no! If I override the ts->pes_payload_size then when not using the > interleaved some unused memory will be allocated. > Please note that when using the sequential mode the payload is writed > "directly" to the output stream. And this is true with PES packets with > a size greater than pes_payload_size (i.e. video). So the > ts->pes_payload_size is used only to store small writes until a PES > packet is complete. However, when using the interleaved mode the entire > PES packet requires to be saved until the last part is writed in the > output stream. For this reason the malloc has different values. I don't understand your reasoning. Sequential mode is not affected if you only increase pes_payload_size if parallel mode is used. Is parallel mode affected? >> > > > + for (i = 0; i < ts->nb_services; i++) { >> > > > + service = ts->services[i]; >> > > > ... >> > > > } >> > > > >> > > >> > > Why do you need the loop over the services here? It seems unrelated unless >> > > I missed something. >> > >> > I explained it in my original message... >> > The current code has a bug and it doesn't set correctly the value of the >> > service->pcr_pid. And this patch requires correct values of pcr_pid for each >> > program. Please note that this patch makes complete sense when mixing multiple >> > video streams, such as when using multiple programs. >> >> This should be a separate patch then. I suggest implementing this before >> adding interleaving support. > > Yes and not... > > 1. This interleaved mode requires correct value of the service->pcr_pid, as it have > sense with multiple programs. So the "patch" here is mandatory. We can assume that > it's part of the new mux mode. > > 2. With the sequential mode, the service->pcr_pid bug is hidden, because no one uses > multiple programs. So a lot of work for me for an irrelevant fix. > > So I'd prefer to just comment here, and make it all one single change. We generally split features to different patches to make patch review easier. Or to put it in a different way, your patch has bigger chance to be reviewed and merged if it touches a single feature. I see no reason why one couldn't use multiple programs in sequential mode so it totally makes sense to implement it, and then implement parallel mode on top of that as a separete patch. > > >> > > > + - NOTE: 'payload' contains a complete PES payload. >> > > > + - NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); >> > > > + - When 'mode' = -1 it writes the entire payload with the header; >> > > > + - when 'mode' = 0 it writes only one TS packet with the header; >> > > > + - when 'mode' > 0 it writes only one TS packet. >> > > > >> > > >> > > Enum for mode would make more sense to me >> > >> > Yes and not. The code is more compact in this case with numerical values, as you >> > don't need to do checks when you call to the function. >> >> The code might be more compact, and tons of less readable. Use enums, or >> defines. You might also use flags for mode, if that makes the code more >> readable. > > > I'm sorry, no. In my opinion it's more legible and compact like that. > I accept all your comments, and I appreciate them. But in this case I do not > see the advantage. I disagree. Based on your code the 4 modes do this: < -1 = 0 -1 = WRITE_HEADER 0 = WRITE_HEADER | ONE_PACKET_ONLY > 0 = ONE_PACKET_ONLY You documented the meaning of the "modes" in two places and created two helper functions to make sure you pass the correct modes to the underlying function. Why is that if not because you can't seem to remember yourself the meaning of modes? Please reconsider using defines or enums. Thanks, Marton
On 6/30/19, Marton Balint <cus@passwd.hu> wrote: > > > On Wed, 26 Jun 2019, Andreas Håkon wrote: > >>> > > > - ts_st->payload = av_mallocz(ts->pes_payload_size); >>> > > > + ts_st->payload = av_mallocz(ts->parallel_mux ? >>> > > > MAX_PES_PAYLOAD : ts->pes_payload_size); >>> > > >>> > > Could you clarify why this needs to be changed? >>> > >>> > Sure! Because when you write in parallel you're delaying the writing of >>> > the PES packet. >>> > So you need to save the entire PES packet. And the ts->pes_payload_size >>> > it's defined to >>> > a very small value (2734 Bytes only)... See at the top of the >>> > mpegtsenc.c file: >>> > #define DEFAULT_PES_HEADER_FREQ 16 >>> > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + >>> > 170) >>> >>> OK, but you shold override ts->pes_payload_size, and not the malloc, no? >>> There is code in mpegts_init which sets pes_payload_size, you should >>> force >>> it to MAX_PES_PAYLOAD there before doing the round up. >> >> No, no! If I override the ts->pes_payload_size then when not using the >> interleaved some unused memory will be allocated. >> Please note that when using the sequential mode the payload is writed >> "directly" to the output stream. And this is true with PES packets with >> a size greater than pes_payload_size (i.e. video). So the >> ts->pes_payload_size is used only to store small writes until a PES >> packet is complete. However, when using the interleaved mode the entire >> PES packet requires to be saved until the last part is writed in the >> output stream. For this reason the malloc has different values. > > I don't understand your reasoning. Sequential mode is not affected if you > only increase pes_payload_size if parallel mode is used. Is parallel > mode affected? > >>> > > > + for (i = 0; i < ts->nb_services; i++) { >>> > > > + service = ts->services[i]; >>> > > > ... >>> > > > } >>> > > > >>> > > >>> > > Why do you need the loop over the services here? It seems unrelated >>> > > unless >>> > > I missed something. >>> > >>> > I explained it in my original message... >>> > The current code has a bug and it doesn't set correctly the value of >>> > the >>> > service->pcr_pid. And this patch requires correct values of pcr_pid for >>> > each >>> > program. Please note that this patch makes complete sense when mixing >>> > multiple >>> > video streams, such as when using multiple programs. >>> >>> This should be a separate patch then. I suggest implementing this before >>> adding interleaving support. >> >> Yes and not... >> >> 1. This interleaved mode requires correct value of the service->pcr_pid, >> as it have >> sense with multiple programs. So the "patch" here is mandatory. We can >> assume that >> it's part of the new mux mode. >> >> 2. With the sequential mode, the service->pcr_pid bug is hidden, because >> no one uses >> multiple programs. So a lot of work for me for an irrelevant fix. >> >> So I'd prefer to just comment here, and make it all one single change. > > We generally split features to different patches to make patch review > easier. Or to put it in a different way, your patch has bigger chance to > be reviewed and merged if it touches a single feature. > > I see no reason why one couldn't use multiple programs in sequential mode > so it totally makes sense to implement it, and then implement parallel > mode on top of that as a separete patch. > >> >> >>> > > > + - NOTE: 'payload' contains a complete PES payload. >>> > > > + - NOTE2: When 'mode' < -1 it writes the rest of the payload >>> > > > (without header); >>> > > > + - When 'mode' = -1 it writes the entire payload with >>> > > > the header; >>> > > > + - when 'mode' = 0 it writes only one TS packet with >>> > > > the header; >>> > > > + - when 'mode' > 0 it writes only one TS packet. >>> > > > >>> > > >>> > > Enum for mode would make more sense to me >>> > >>> > Yes and not. The code is more compact in this case with numerical >>> > values, as you >>> > don't need to do checks when you call to the function. >>> >>> The code might be more compact, and tons of less readable. Use enums, or >>> defines. You might also use flags for mode, if that makes the code more >>> readable. >> >> >> I'm sorry, no. In my opinion it's more legible and compact like that. >> I accept all your comments, and I appreciate them. But in this case I do >> not >> see the advantage. > > I disagree. Based on your code the 4 modes do this: > > < -1 = 0 > -1 = WRITE_HEADER > 0 = WRITE_HEADER | ONE_PACKET_ONLY >> 0 = ONE_PACKET_ONLY > > You documented the meaning of the "modes" in two places and created two > helper functions to make sure you pass the correct modes to the underlying > function. Why is that if not because you can't seem to remember yourself > the meaning of modes? Please reconsider using defines or enums. > Hardcoded values are unacceptable. Every such patch will not be applied and if someone applies it by accident it will be reverted ASAP.
Hi Marton, > > > > > > - ts_st->payload = av_mallocz(ts->pes_payload_size); > > > > > > + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); > > > > > > > > > > > > > > > > Could you clarify why this needs to be changed? > > > > > > > > Sure! Because when you write in parallel you're delaying the writing of the PES packet. > > > > So you need to save the entire PES packet. And the ts->pes_payload_size it's defined to > > > > a very small value (2734 Bytes only)... See at the top of the mpegtsenc.c file: > > > > #define DEFAULT_PES_HEADER_FREQ 16 > > > > #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) > > > > > > OK, but you shold override ts->pes_payload_size, and not the malloc, no? > > > There is code in mpegts_init which sets pes_payload_size, you should force > > > it to MAX_PES_PAYLOAD there before doing the round up. > > > > No, no! If I override the ts->pes_payload_size then when not using the > > interleaved some unused memory will be allocated. > > Please note that when using the sequential mode the payload is writed > > "directly" to the output stream. And this is true with PES packets with > > a size greater than pes_payload_size (i.e. video). So the > > ts->pes_payload_size is used only to store small writes until a PES > > packet is complete. However, when using the interleaved mode the entire > > PES packet requires to be saved until the last part is writed in the > > output stream. For this reason the malloc has different values. > > I don't understand your reasoning. Sequential mode is not affected if you > only increase pes_payload_size if parallel mode is used. Is parallel > mode affected? Don't worry! First of all three points: 1. For the Sequential mode we CAN'T change the value of pes_payload_size. 2. For the Sequential mode we CAN increase the size of the malloc for the ts_st->payload. But this is useless (a wasted allocated space). 3. If we increase the size of ts->pes_payload_size this will affect to both modes. Why? Perhaps your problem is this: the value of ts->pes_payload_size in the current code doesn't represents the MAX payload size of a PES message, but the MINIMUM size for a PES message. However, for the new Parallel mode, we need to save the total PES message until the last part of it is writed. And for this objective we save it in the ts_ts->payload; and the size in this case requires to be the MAX_PES_PAYLOAD. Futhermore, if we increase the size of ts->pes_payload_size this will affect to both modes. Just in one line: "ts->pes_payload_size" IS NOT the size of the PES packet !!! More clear now? > > > > > > - for (i = 0; i < ts->nb_services; i++) { > > > > > > - service = ts->services[i]; > > > > > > > > > > > > > > > > > > ... > > > > > > } > > > > > > > > > > > > > > > > Why do you need the loop over the services here? It seems unrelated unless > > > > > I missed something. > > > > > > > > I explained it in my original message... > > > > The current code has a bug and it doesn't set correctly the value of the > > > > service->pcr_pid. And this patch requires correct values of pcr_pid for each > > > > program. Please note that this patch makes complete sense when mixing multiple > > > > video streams, such as when using multiple programs. > > > > > > This should be a separate patch then. I suggest implementing this before > > > adding interleaving support. > > > > Yes and not... > > > > 1. This interleaved mode requires correct value of the service->pcr_pid, as it have > > sense with multiple programs. So the "patch" here is mandatory. We can assume that > > it's part of the new mux mode. > > > > 2. With the sequential mode, the service->pcr_pid bug is hidden, because no one uses > > multiple programs. So a lot of work for me for an irrelevant fix. > > > > > > So I'd prefer to just comment here, and make it all one single change. > > We generally split features to different patches to make patch review > easier. Or to put it in a different way, your patch has bigger chance to > be reviewed and merged if it touches a single feature. I understand it. However, my personal objective is to make this patch fit the criteria for acceptance. I published here other patches, but several of them are not accepted. I don't want to discuss your criteria, and I prefer to accept it. However, I don't have the time to fix something that I don't need it. So, I pointed it here as the bug exists and this patch fixes it. > I see no reason why one couldn't use multiple programs in sequential mode > so it totally makes sense to implement it, and then implement parallel > mode on top of that as a separete patch. Sure, but if this patch is accepted the bug is resolved too. > > > > > > - - NOTE: 'payload' contains a complete PES payload. > > > > > > - - NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); > > > > > > - - When 'mode' = -1 it writes the entire payload with the header; > > > > > > > > > > > > > > > > > > - - when 'mode' = 0 it writes only one TS packet with the header; > > > > > > > > > > > > > > > > > > - - when 'mode' > 0 it writes only one TS packet. > > > > > > > > > > > > > > > > > > > > > > Enum for mode would make more sense to me > > > > > > > > Yes and not. The code is more compact in this case with numerical values, as you > > > > don't need to do checks when you call to the function. > > > > > > The code might be more compact, and tons of less readable. Use enums, or > > > defines. You might also use flags for mode, if that makes the code more > > > readable. > > > > I'm sorry, no. In my opinion it's more legible and compact like that. > > I accept all your comments, and I appreciate them. But in this case I do not > > see the advantage. > > I disagree. Based on your code the 4 modes do this: > > < -1 = 0 > -1 = WRITE_HEADER > 0 = WRITE_HEADER | ONE_PACKET_ONLY > > 0 = ONE_PACKET_ONLY > > You documented the meaning of the "modes" in two places and created two > helper functions to make sure you pass the correct modes to the underlying > function. Why is that if not because you can't seem to remember yourself > the meaning of modes? Please reconsider using defines or enums. Sorry! I think you're mixing several different topics: The code pointed here DON'T HAVE ANY DIRECT RELATION with the previous comment in the header of the function "mpegts_write_pes()". This code is internal in the function "write_side_streams()". This last function uses an internal variable write_mode that is used in the case switch to make the code less complex. Note that the function receives the parameter "parallel". So the value of the "write_mode" is calculated in multiple parts of the internal code. If I remove this simple "internal" comments I'm sure you don't see any relation between them. I prefer to stop commenting about the NUMERICAL value of the parameter "mode" in the function "mpegts_write_pes()". If it's really required I'll change it to a DEFINE value. In any case, please continue with the new version of the patch posted days ago: https://patchwork.ffmpeg.org/patch/13745/ Regards. A.H. ---
Hi Paul, > > I disagree. Based on your code the 4 modes do this: > > < -1 = 0 > > -1 = WRITE_HEADER > > 0 = WRITE_HEADER | ONE_PACKET_ONLY > > > > > 0 = ONE_PACKET_ONLY > > > > You documented the meaning of the "modes" in two places and created two > > helper functions to make sure you pass the correct modes to the underlying > > function. Why is that if not because you can't seem to remember yourself > > the meaning of modes? Please reconsider using defines or enums. > > Hardcoded values are unacceptable. Every such patch will not be applied > and if someone applies it by accident it will be reverted ASAP. The code doesn't use any hardcoded value! What Marton is commenting here it's a mistake of him. The variable "write_mode" inside the function "write_side_streams()" of the patch is a COUNTER. He thought it had something to do with a comment on the parameter "mode" in the function "mpegts_write_pes()". But is false. In any case, I'll change the value of the parameter "mode" in "mpegts_write_pes()". Please, note that a new version of the patch is ready published: https://patchwork.ffmpeg.org/patch/13745/ Regards. A.H. ---
Hi Paul, Marton and Andriy, I have updated the patch following the comments. The first part that solves only the PCR bug with multiple programs is already published: https://patchwork.ffmpeg.org/patch/14036/ I hope it will soon be accepted. Regards. A.H. ---
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index fc0ea22..93f8d3d 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -97,6 +97,7 @@ typedef struct MpegTSWrite { int pmt_start_pid; int start_pid; int m2ts_mode; + int parallel_mux; int reemit_pat_pmt; // backward compatibility @@ -120,6 +121,7 @@ typedef struct MpegTSWrite { /* a PES packet header is generated every DEFAULT_PES_HEADER_FREQ packets */ #define DEFAULT_PES_HEADER_FREQ 16 #define DEFAULT_PES_PAYLOAD_SIZE ((DEFAULT_PES_HEADER_FREQ - 1) * 184 + 170) +#define MAX_PES_PAYLOAD 2 * 200 * 1024 // From mpegts.c /* The section length is 12 bits. The first 2 are set to 0, the remaining * 10 bits should not exceed 1021. */ @@ -227,17 +229,28 @@ static int mpegts_write_section1(MpegTSSection *s, int tid, int id, #define PAT_RETRANS_TIME 100 #define PCR_RETRANS_TIME 20 +#define PES_START 1 /* 0000 0001 */ +#define PES_FULL 2 /* 0000 0010 */ +#define UNUSED_FLAG_3 4 /* 0000 0100 */ +#define UNUSED_FLAG_4 8 /* 0000 1000 */ +#define UNUSED_FLAG_5 16 /* 0001 0000 */ +#define UNUSED_FLAG_6 32 /* 0010 0000 */ +#define UNUSED_FLAG_7 64 /* 0100 0000 */ +#define PES_NEEDS_END 128 /* 1000 0000 */ + typedef struct MpegTSWriteStream { struct MpegTSService *service; int pid; /* stream associated pid */ int cc; int discontinuity; int payload_size; + int payload_top; int first_pts_check; ///< first pts check needed int prev_payload_key; int64_t payload_pts; int64_t payload_dts; int payload_flags; + int pes_flags; uint8_t *payload; AVFormatContext *amux; AVRational user_tb; @@ -866,7 +879,7 @@ static int mpegts_init(AVFormatContext *s) ts_st->user_tb = st->time_base; avpriv_set_pts_info(st, 33, 1, 90000); - ts_st->payload = av_mallocz(ts->pes_payload_size); + ts_st->payload = av_mallocz(ts->parallel_mux ? MAX_PES_PAYLOAD : ts->pes_payload_size); if (!ts_st->payload) { ret = AVERROR(ENOMEM); goto fail; @@ -910,6 +923,8 @@ static int mpegts_init(AVFormatContext *s) pids[i] = ts_st->pid; ts_st->payload_pts = AV_NOPTS_VALUE; ts_st->payload_dts = AV_NOPTS_VALUE; + ts_st->payload_top = 0; + ts_st->pes_flags = 0; ts_st->first_pts_check = 1; ts_st->cc = 15; ts_st->discontinuity = ts->flags & MPEGTS_FLAG_DISCONT; @@ -953,46 +968,52 @@ static int mpegts_init(AVFormatContext *s) 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 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); + } else { + service->pcr_packet_period = + 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; } ts->last_pat_ts = AV_NOPTS_VALUE; @@ -1005,8 +1026,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; @@ -1172,9 +1191,14 @@ static uint8_t *get_ts_payload_start(uint8_t *pkt) /* Add a PES header to the front of the payload, and segment into an integer * number of TS packets. The final TS packet is padded using an oversized * adaptation header to exactly fill the last TS packet. - * NOTE: 'payload' contains a complete PES payload. */ -static void mpegts_write_pes(AVFormatContext *s, AVStream *st, - const uint8_t *payload, int payload_size, + * NOTE: 'payload' contains a complete PES payload. + * NOTE2: When 'mode' < -1 it writes the rest of the payload (without header); + * When 'mode' = -1 it writes the entire payload with the header; + * when 'mode' = 0 it writes only one TS packet with the header; + * when 'mode' > 0 it writes only one TS packet. + * Note3: It returns the number of writed bytes. */ +static int mpegts_write_pes(AVFormatContext *s, AVStream *st, + const uint8_t *payload, int payload_size, int mode, int64_t pts, int64_t dts, int key, int stream_id) { MpegTSWriteStream *ts_st = st->priv_data; @@ -1186,13 +1210,14 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, int64_t pcr = -1; /* avoid warning */ int64_t delay = av_rescale(s->max_delay, 90000, AV_TIME_BASE); int force_pat = st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO && key && !ts_st->prev_payload_key; + int ret_size = 0; av_assert0(ts_st->payload != buf || st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO); if (ts->flags & MPEGTS_FLAG_PAT_PMT_AT_FRAMES && st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { force_pat = 1; } - is_start = 1; + is_start = (mode > 0 || mode < -1) ? 0 : 1; while (payload_size > 0) { retransmit_si_info(s, force_pat, dts); force_pat = 0; @@ -1389,6 +1414,8 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, memset(q, 0xff, pes_header_stuffing_bytes); q += pes_header_stuffing_bytes; } + if (is_dvb_subtitle) + ts_st->pes_flags |= PES_NEEDS_END; is_start = 0; } /* header size */ @@ -1420,7 +1447,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, } } - if (is_dvb_subtitle && payload_size == len) { + if ((ts_st->pes_flags & PES_NEEDS_END) && payload_size == len) { memcpy(buf + TS_PACKET_SIZE - len, payload, len - 1); buf[TS_PACKET_SIZE - 1] = 0xff; /* end_of_PES_data_field_marker: an 8-bit field with fixed contents 0xff for DVB subtitle */ } else { @@ -1431,8 +1458,12 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, payload_size -= len; mpegts_prefix_m2ts_header(s); avio_write(s->pb, buf, TS_PACKET_SIZE); + ret_size += len; + if (mode >= 0) + payload_size = 0; // Write one packet only then exit } ts_st->prev_payload_key = key; + return ret_size; } int ff_check_h264_startcode(AVFormatContext *s, const AVStream *st, const AVPacket *pkt) @@ -1519,6 +1550,102 @@ static int opus_get_packet_samples(AVFormatContext *s, AVPacket *pkt) return duration; } +static inline int write_full_pes_stream(AVFormatContext *s, AVStream *st, + const uint8_t *payload, int payload_size, + int64_t pts, int64_t dts, int key, int stream_id) +{ + return mpegts_write_pes(s, st, payload, payload_size, -1, pts, dts, key, stream_id); +} + +static inline int write_flush_pes_stream(AVFormatContext *s, AVStream *st, + const uint8_t *payload, int payload_size, + int64_t pts, int64_t dts, int key, int stream_id) +{ + return mpegts_write_pes(s, st, payload, payload_size, -2, pts, dts, key, stream_id); +} + +static inline int write_pkt_pes_stream(AVFormatContext *s, AVStream *st, + const uint8_t *payload, int payload_size, int mode, + int64_t pts, int64_t dts, int key, int stream_id) +{ + return mpegts_write_pes(s, st, payload, payload_size, mode, pts, dts, key, stream_id); +} + +static int write_side_streams(AVFormatContext *s, int64_t dts, int64_t delay, int stream_id, int parallel) +{ + int i; + int sum = 0; + int check_mode; + int write_mode; // -1: Don't write anything + // 0: Start the PES packet and write 1 TS packet + // 1: Continue writing just 1 TS packet + // 2: Write full PES packet + int ret; + for(i=0; i<s->nb_streams; i++) { + AVStream *st2 = s->streams[i]; + MpegTSWriteStream *ts_st2 = st2->priv_data; + + check_mode = parallel ? (ts_st2->payload_top > 0) : 0; + + if (ts_st2->payload_size && !check_mode + && (ts_st2->payload_dts == AV_NOPTS_VALUE || dts - ts_st2->payload_dts > delay/2 || (ts_st2->pes_flags & PES_START))) { + write_mode = parallel ? 0 : 2; + } else if (check_mode) { + write_mode = 1; + } else { + write_mode = -1; + } + + switch (write_mode) + { + case 2: // SEQUENTIAL MODE + ret = write_full_pes_stream(s, st2, ts_st2->payload, ts_st2->payload_size, + ts_st2->payload_pts, ts_st2->payload_dts, + ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); + ts_st2->payload_size = 0; + sum += ret; + break; + case 1: // PARALLEL MODE + case 0: + ret = write_pkt_pes_stream(s, st2, ts_st2->payload + ts_st2->payload_top, + ts_st2->payload_size - ts_st2->payload_top, write_mode, + ts_st2->payload_pts, ts_st2->payload_dts, + ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); + ts_st2->payload_top += ret; + sum += ret; + break; + default: + continue; + } + + if (ts_st2->payload_size && ts_st2->payload_top == ts_st2->payload_size) { + ts_st2->payload_size = 0; + ts_st2->payload_top = 0; + ts_st2->pes_flags = 0; + } + } + return sum; +} + +/* NOTE: The return value is negative when the onlyone flag generates the exit. */ +static int write_with_side_streams(AVFormatContext *s, AVStream *st, int payload_top, + const uint8_t *payload, int payload_size, + int64_t pts, int64_t dts, int64_t delay, int key, int stream_id) +{ + int force = payload_size - payload_top > MAX_PES_PAYLOAD ? 1 : 0; + if (force) + av_log(s, AV_LOG_WARNING, "PES packet oversized, full sequential writing required\n"); + if (payload_size && payload_top != payload_size) { + do { + payload_top += mpegts_write_pes(s, st, payload + payload_top, + payload_size - payload_top, payload_top ? 1 : 0, + pts, dts, key, stream_id); + } while (force && payload_size != payload_top); + } + write_side_streams(s, dts, delay, stream_id, 1); + return payload_top; +} + static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) { AVStream *st = s->streams[pkt->stream_index]; @@ -1739,53 +1866,77 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) } } - if (pkt->dts != AV_NOPTS_VALUE) { - int i; - for(i=0; i<s->nb_streams; i++) { - AVStream *st2 = s->streams[i]; - MpegTSWriteStream *ts_st2 = st2->priv_data; - if ( ts_st2->payload_size - && (ts_st2->payload_dts == AV_NOPTS_VALUE || dts - ts_st2->payload_dts > delay/2)) { - mpegts_write_pes(s, st2, ts_st2->payload, ts_st2->payload_size, - ts_st2->payload_pts, ts_st2->payload_dts, - ts_st2->payload_flags & AV_PKT_FLAG_KEY, stream_id); - ts_st2->payload_size = 0; - } - } + // Write pending PES packets (SEQUENTIAL MODE) + if (pkt->dts != AV_NOPTS_VALUE && !ts->parallel_mux) { + write_side_streams(s, dts, delay, stream_id, 0); } + // Complete a new PES packet (new incoming data will go into another PES) if (ts_st->payload_size && (ts_st->payload_size + size > ts->pes_payload_size || - (dts != AV_NOPTS_VALUE && ts_st->payload_dts != AV_NOPTS_VALUE && - av_compare_ts(dts - ts_st->payload_dts, st->time_base, - s->max_delay, AV_TIME_BASE_Q) >= 0) || - ts_st->opus_queued_samples + opus_samples >= 5760 /* 120ms */)) { - mpegts_write_pes(s, st, ts_st->payload, ts_st->payload_size, - ts_st->payload_pts, ts_st->payload_dts, - ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); - ts_st->payload_size = 0; - ts_st->opus_queued_samples = 0; + (dts != AV_NOPTS_VALUE && ts_st->payload_dts != AV_NOPTS_VALUE && + av_compare_ts(dts - ts_st->payload_dts, st->time_base, + s->max_delay, AV_TIME_BASE_Q) >= 0) || + ts_st->opus_queued_samples + opus_samples >= 5760 /* 120ms */)) { + if (!ts->parallel_mux || ts_st->opus_queued_samples) { + write_full_pes_stream(s, st, ts_st->payload, ts_st->payload_size, + ts_st->payload_pts, ts_st->payload_dts, + ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); + ts_st->payload_size = 0; + ts_st->opus_queued_samples = 0; + } else { + ts_st->pes_flags |= PES_START | PES_FULL; + } + } + + // Write pending PES packets (PARALLEL MODE) + if (pkt->dts != AV_NOPTS_VALUE && ts->parallel_mux && (ts_st->pes_flags & PES_START)) { + // Empty previous pending PES packet before starting a new one or write one packet + do { + write_with_side_streams(s, st, 0, + ts_st->payload, 0, + ts_st->payload_pts, ts_st->payload_dts, delay, + ts_st->payload_flags & AV_PKT_FLAG_KEY, stream_id); + } while (ts_st->pes_flags & PES_START); } + // Directly write a new PES packet with the incoming data if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO || size > ts->pes_payload_size) { av_assert0(!ts_st->payload_size); // for video and subtitle, write a single pes packet - mpegts_write_pes(s, st, buf, size, pts, dts, - pkt->flags & AV_PKT_FLAG_KEY, stream_id); - ts_st->opus_queued_samples = 0; - av_free(data); - return 0; + if (!ts->parallel_mux || ts_st->opus_queued_samples) { + write_full_pes_stream(s, st, buf, size, pts, dts, + pkt->flags & AV_PKT_FLAG_KEY, stream_id); + ts_st->opus_queued_samples = 0; + goto free; + } else { + ts_st->payload_top = write_with_side_streams(s, st, 0, + buf, size, + pts, dts, delay, + pkt->flags & AV_PKT_FLAG_KEY, stream_id); + if (ts_st->payload_top == size) { + ts_st->payload_size = 0; + ts_st->payload_top = 0; + ts_st->pes_flags = 0; + goto free; + } + ts_st->pes_flags |= PES_START; + ts_st->payload_size = 0; // this value will be set later + } } + // Start a new PES packet if (!ts_st->payload_size) { ts_st->payload_pts = pts; ts_st->payload_dts = dts; ts_st->payload_flags = pkt->flags; } + // Enqueue data in the current PES packet memcpy(ts_st->payload + ts_st->payload_size, buf, size); ts_st->payload_size += size; ts_st->opus_queued_samples += opus_samples; +free: av_free(data); return 0; @@ -1794,13 +1945,20 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) static void mpegts_write_flush(AVFormatContext *s) { int i; + MpegTSWrite *ts = s->priv_data; /* flush current packets */ for (i = 0; i < s->nb_streams; i++) { AVStream *st = s->streams[i]; MpegTSWriteStream *ts_st = st->priv_data; if (ts_st->payload_size > 0) { - mpegts_write_pes(s, st, ts_st->payload, ts_st->payload_size, + if (!ts->parallel_mux || ts_st->payload_top == 0) + write_full_pes_stream(s, st, ts_st->payload, ts_st->payload_size, + ts_st->payload_pts, ts_st->payload_dts, + ts_st->payload_flags & AV_PKT_FLAG_KEY, -1); + else + write_flush_pes_stream(s, st, ts_st->payload + ts_st->payload_top, + ts_st->payload_size - ts_st->payload_top, ts_st->payload_pts, ts_st->payload_dts, ts_st->payload_flags & AV_PKT_FLAG_KEY, -1); ts_st->payload_size = 0; @@ -1920,6 +2078,9 @@ static const AVOption options[] = { { "mpegts_m2ts_mode", "Enable m2ts mode.", offsetof(MpegTSWrite, m2ts_mode), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, AV_OPT_FLAG_ENCODING_PARAM }, + { "mpegts_extra_mux", "Enable Non-Sequential muxing mode.", + offsetof(MpegTSWrite, parallel_mux), AV_OPT_TYPE_BOOL, + { .i64 = 0 }, 0, 1, AV_OPT_FLAG_ENCODING_PARAM }, { "muxrate", NULL, offsetof(MpegTSWrite, mux_rate), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM },