diff mbox series

[FFmpeg-devel,v3,1/3] avformat/mpegtsenc: Fix mpegts_write_pes() for private_stream_2 and other types

Message ID 20210422120311.10294-1-xqq@xqq.im
State Accepted
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
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(+)

Comments

Marton Balint April 24, 2021, 5:13 p.m. UTC | #1
On Thu, 22 Apr 2021, 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(+)

The order of the patches seems wrong. Patch/3 (or similar) should be 
applied first, otherwise you are checking the stream_id provided via side 
data, not the stream id actually used.

Also note that is_dvb_teletext/is_dvb_subtitle might not be set if certain 
stream_id is used, that can also cause issues.

I will send a patch series for these two issues, please check them and if 
they look good then rebase your patchset on top of that.

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

You should add the relevant stream type constants to mpegts.h and use 
those for the checks, you can spare the comments that way.

> +
>             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 */

Thanks,
Marton
zheng qian April 25, 2021, 3:29 a.m. UTC | #2
On Sun, Apr 25, 2021 at 2:13 AM Marton Balint <cus@passwd.hu> wrote:
>
> The order of the patches seems wrong. Patch/3 (or similar) should be
> applied first, otherwise you are checking the stream_id provided via side
> data, not the stream id actually used.
>
> Also note that is_dvb_teletext/is_dvb_subtitle might not be set if certain
> stream_id is used, that can also cause issues.
>
> I will send a patch series for these two issues, please check them and if
> they look good then rebase your patchset on top of that.
>
> You should add the relevant stream type constants to mpegts.h and use
> those for the checks, you can spare the comments that way.
>

I have sent a new patchset v4 for these changes.
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=3833

Notice that I re-sent the 3/4 patch file which seems to be lost,
so please pay attention to the order.

Thanks,
zheng
Marton Balint April 28, 2021, 7:39 p.m. UTC | #3
On Sun, 25 Apr 2021, zheng qian wrote:

> On Sun, Apr 25, 2021 at 2:13 AM Marton Balint <cus@passwd.hu> wrote:
>>
>> The order of the patches seems wrong. Patch/3 (or similar) should be
>> applied first, otherwise you are checking the stream_id provided via side
>> data, not the stream id actually used.
>>
>> Also note that is_dvb_teletext/is_dvb_subtitle might not be set if certain
>> stream_id is used, that can also cause issues.
>>
>> I will send a patch series for these two issues, please check them and if
>> they look good then rebase your patchset on top of that.
>>
>> You should add the relevant stream type constants to mpegts.h and use
>> those for the checks, you can spare the comments that way.
>>
>
> I have sent a new patchset v4 for these changes.
> https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=3833
>
> Notice that I re-sent the 3/4 patch file which seems to be lost,
> so please pay attention to the order.

Thanks, applied the series with slightly modified/extended commit 
message based on your earlier commit message.

Regards,
Marton
zheng qian April 29, 2021, 12:09 a.m. UTC | #4
On Thu, Apr 29, 2021 at 4:40 AM Marton Balint <cus@passwd.hu> wrote:
>
> Thanks, applied the series with slightly modified/extended commit
> message based on your earlier commit message.
>
> Regards,
> Marton

Thank you!

Best regards,
zheng
diff mbox series

Patch

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 */