diff mbox series

[FFmpeg-devel,v3,3/3] avformat/mpegtsenc: Write stream_id into PES after stream_id decision

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
Related show

Checks

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

Commit Message

zheng qian April 22, 2021, 12:03 p.m. UTC
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(-)

Comments

Mao Hata April 24, 2021, 7:45 a.m. UTC | #1
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.
zheng qian April 24, 2021, 9:35 a.m. UTC | #2
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 mbox series

Patch

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