| Message ID | 20210422120311.10294-3-xqq@xqq.im |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3,1/3] avformat/mpegtsenc: Fix mpegts_write_pes() for private_stream_2 and other types | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 2021/04/22 21:03, zheng qian wrote: > Changes since v2: > Fix PES_packet_length mismatch bug > > According to the PES packet definition defined in Table 2-17 > of ISO_IEC_13818-1 specification, some fields like PTS/DTS or > pes_extension could only appears if the stream_id meets the > condition: > > if (stream_id != 0xBC && // program_stream_map > stream_id != 0xBE && // padding_stream > stream_id != 0xBF && // private_stream_2 > stream_id != 0xF0 && // ECM > stream_id != 0xF1 && // EMM > stream_id != 0xFF && // program_stream_directory > stream_id != 0xF2 && // DSMCC_stream > stream_id != 0xF8) // ITU-T Rec. H.222.1 type E stream > > And the following stream_id types don't have fields like PTS/DTS: > > else if ( stream_id == program_stream_map > || stream_id == private_stream_2 > || stream_id == ECM > || stream_id == EMM > || stream_id == program_stream_directory > || stream_id == DSMCC_stream > || stream_id == ITU-T Rec. H.222.1 type E stream ) { > for (i = 0; i < PES_packet_length; i++) { > PES_packet_data_byte > } > } > > Current implementation skipped the judgement of stream_id > causing some kind of stream like private_stream_2 to be > incorrectly written with PTS/DTS field. For example, > Japan DTV transmits news and alerts through ARIB superimpose > that utilizes private_stream_2 still could not be remuxed > correctly for now. > > This patch set fixes the remuxing for private_stream_2 and > other stream_id types. > > Signed-off-by: zheng qian <xqq@xqq.im> > --- > libavformat/mpegtsenc.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index f302af84ff..b59dab5174 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > @@ -1476,6 +1476,16 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > } > } > header_len = 0; > + > + if (stream_id != 0xBC && // program_stream_map > + stream_id != 0xBE && // padding_stream > + stream_id != 0xBF && // private_stream_2 > + stream_id != 0xF0 && // ECM > + stream_id != 0xF1 && // EMM > + stream_id != 0xFF && // program_stream_directory > + stream_id != 0xF2 && // DSMCC_stream > + stream_id != 0xF8) { // ITU-T Rec. H.222.1 type E stream > + > flags = 0; > if (pts != AV_NOPTS_VALUE) { > header_len += 5; > @@ -1569,6 +1579,11 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, > memset(q, 0xff, pes_header_stuffing_bytes); > q += pes_header_stuffing_bytes; > } > + } else { > + len = payload_size; > + *q++ = len >> 8; > + *q++ = len; > + } > is_start = 0; > } > /* header size */ > Your patch changes the behavior of mpegts_write_pes() function only if its "st->codecpar->codec_type" parameter is AVMEDIA_TYPE_DATA. In other cases, "stream_id" variable in this function is set to any of the following: STREAM_ID_EXTENDED_STREAM_ID (0xfd) STREAM_ID_VIDEO_STREAM_0 (0xe0) STREAM_ID_AUDIO_STREAM_0 (0xc0) STREAM_ID_EXTENDED_STREAM_ID (0xfd) STREAM_ID_PRIVATE_STREAM_1 (0xbd) So, the "if (stream_id != ...)" statement added by the patch is always true, therefore, the function behaves the same as before. If "st->codecpar->codec_type" parameter is AVMEDIA_TYPE_DATA, unless additionally conditioned by "st->codecpar->codec_id" parameter, basically, the "stream_id" value given as an argument is written to PES packets as it is. In this case, the behavior of the function should be fixed as you commented. v3 patch looks good to me. Then, since the patch itself is based on ISO_IEC_13818-1, it perhaps has some (side) effects for ATSC/DVB as well. If possible, you might want to wait for opinions from ATSC/DVB sides. Mao Hata P.S. I confirmed the effect of your patch with the following command. $ ffmpeg -i some_arib_mpeg.ts -c:v libx264 -map 0:v -c:a aac -map 0:a -map 0:4 dest.ts Here "-map 0:4" points to private_stream_2 (ARIB-superimpose). The command now properly remuxes "-map 0:4" stream into "dest.ts" without any editing.
On Sat, Apr 24, 2021 at 5:09 PM Mao Hata <xt4ubq@gmail.com> wrote: > > > v3 patch looks good to me. > Then, since the patch itself is based on ISO_IEC_13818-1, it perhaps has > some (side) effects for ATSC/DVB as well. If possible, you might want to > wait for opinions from ATSC/DVB sides. > > Mao Hata > I will add arib_superimpose codec later once this patchset got merged. Just waiting for other maintainers to review and merge this, yet. Regards, zheng > P.S. > I confirmed the effect of your patch with the following command. > $ ffmpeg -i some_arib_mpeg.ts -c:v libx264 -map 0:v -c:a aac -map 0:a > -map 0:4 dest.ts > > Here "-map 0:4" points to private_stream_2 (ARIB-superimpose). > The command now properly remuxes "-map 0:4" stream into "dest.ts" > without any editing. > _______________________________________________ > 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".
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 967f98931d..525c68ffd1 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -1445,28 +1445,28 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, is_dvb_teletext = 0; if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { if (st->codecpar->codec_id == AV_CODEC_ID_DIRAC) - *q++ = STREAM_ID_EXTENDED_STREAM_ID; + stream_id = STREAM_ID_EXTENDED_STREAM_ID; else - *q++ = STREAM_ID_VIDEO_STREAM_0; + stream_id = STREAM_ID_VIDEO_STREAM_0; } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && (st->codecpar->codec_id == AV_CODEC_ID_MP2 || st->codecpar->codec_id == AV_CODEC_ID_MP3 || st->codecpar->codec_id == AV_CODEC_ID_AAC)) { - *q++ = STREAM_ID_AUDIO_STREAM_0; + stream_id = STREAM_ID_AUDIO_STREAM_0; } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO && st->codecpar->codec_id == AV_CODEC_ID_AC3 && ts->m2ts_mode) { - *q++ = STREAM_ID_EXTENDED_STREAM_ID; + stream_id = STREAM_ID_EXTENDED_STREAM_ID; } else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA && st->codecpar->codec_id == AV_CODEC_ID_TIMED_ID3) { - *q++ = STREAM_ID_PRIVATE_STREAM_1; + stream_id = STREAM_ID_PRIVATE_STREAM_1; } else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA) { - *q++ = stream_id != -1 ? stream_id : STREAM_ID_METADATA_STREAM; + stream_id = stream_id != -1 ? stream_id : STREAM_ID_METADATA_STREAM; if (stream_id == STREAM_ID_PRIVATE_STREAM_1) /* asynchronous KLV */ pts = dts = AV_NOPTS_VALUE; } else { - *q++ = STREAM_ID_PRIVATE_STREAM_1; + stream_id = STREAM_ID_PRIVATE_STREAM_1; if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) { if (st->codecpar->codec_id == AV_CODEC_ID_DVB_SUBTITLE) { is_dvb_subtitle = 1; @@ -1475,6 +1475,7 @@ static void mpegts_write_pes(AVFormatContext *s, AVStream *st, } } } + *q++ = stream_id; header_len = 0; if (stream_id != 0xBC && // program_stream_map
Since stream_id will have effect on the existences of PES header fields like PTS/DTS, it should be better to guarantee stream_id variable to be identical with exact written value. Signed-off-by: zheng qian <xqq@xqq.im> --- libavformat/mpegtsenc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)