diff mbox series

[FFmpeg-devel] avformat/mpegtsenc: private_stream_1 is not asynchronous KLV

Message ID 68e74195-0397-1a11-cc6e-53519fcd8d87@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/mpegtsenc: private_stream_1 is not asynchronous KLV | expand

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

Mao Hata April 17, 2021, 12:38 p.m. UTC 
According to ISO/IEC 13818-1, private_stream_1 is a synchronous
(has PTS/DTS) stream. Asynchronous one is private_stream_2.
From c1663cbdbbd2178cb199e079ec9bb712d1d757d8 Mon Sep 17 00:00:00 2001
From: Mao Hata <xt4ubq@gmail.com>
Date: Sat, 17 Apr 2021 19:55:22 +0900
Subject: [PATCH] avformat/mpegtsenc: private_stream_1 is not asynchronous KLV

This fixes inappropriately removing PTS/DTS from "bin_data" output.

Signed-off-by: Mao Hata <xt4ubq@gmail.com>
---
 libavformat/mpegts.h    | 1 +
 libavformat/mpegtsenc.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Marton Balint April 18, 2021, 8:25 p.m. UTC | #1 
On Sat, 17 Apr 2021, Mao Hata wrote:

> According to ISO/IEC 13818-1, private_stream_1 is a synchronous
> (has PTS/DTS) stream. Asynchronous one is private_stream_2.

Which section describes this?

Also keep in mind that code was added so that AV_CODEC_ID_SMPTE_KLV is 
handled in mpegts, and SMPTE RP 217 promotes the usage of 
private_stream_1, so I don't think this can be simply changed.

Regards,
Marton
Mao Hata April 19, 2021, 3:44 p.m. UTC | #2 
On 2021/04/19 5:25, Marton Balint wrote:
> 
> 
> On Sat, 17 Apr 2021, Mao Hata wrote:
> 
>> According to ISO/IEC 13818-1, private_stream_1 is a synchronous
>> (has PTS/DTS) stream. Asynchronous one is private_stream_2.
> 
> Which section describes this?
> 
> Also keep in mind that code was added so that AV_CODEC_ID_SMPTE_KLV is 
> handled in mpegts, and SMPTE RP 217 promotes the usage of 
> private_stream_1, so I don't think this can be simply changed.
> 
> Regards,
> Marton

Thank you for your reply. And I was not considering SMPTE (I misread the 
word "KLV" just an abbreviation for "Key-Length-Value"). Oh... I 
withdraw this patch.

The reason I created the patch was to fix the problem: when I transcode 
a transport stream based on ARIB-STD-B24, the PTS/DTS is removed from 
private_stream_1 streams, and, conversely, it is added to private_stream_2.
"ISO/IEC 13818-1 Table 2-17 - PES packet" (sorry, the table number may 
have been shifted) defines the conditions under which PTS/DTS can exist 
as follows:

 >if (stream_id != program_stream_map
 > && stream_id != padding_stream
 > && 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) {

So, private_stream_2 can not insert PTS/DTS. In addition, "ARIB-STD-B24, 
VOLUME3, Chapter5" defines private_stream_1 as "Synchronized PES" 
(mainly used for subtitles, with PTS/DTS inserted) and
private_stream_2 as "Asynchronous PES" (mainly used for superinpose, no 
PTS/DTS inserted).

 From these things, I misunderstood like that " PTS/DTS is generally 
inserted for private_stream_1, so the `if (stream_id == 
STREAM_ID_PRIVATE_STREAM_1)` statement (I patched) must be a mistake! " ...

Anyway, as I read the related code, I now understand that the problem 
should be solved specifically for ARIB-based transport stream (at 
"mpegts.c" or so.).
Thank you again.
zheng qian April 19, 2021, 4:36 p.m. UTC | #3 
On Tue, Apr 20, 2021 at 12:44 AM Mao Hata <xt4ubq@gmail.com> wrote:
>
> The reason I created the patch was to fix the problem: when I transcode
> a transport stream based on ARIB-STD-B24, the PTS/DTS is removed from
> private_stream_1 streams, and, conversely, it is added to private_stream_2.
> "ISO/IEC 13818-1 Table 2-17 - PES packet" (sorry, the table number may
> have been shifted) defines the conditions under which PTS/DTS can exist
> as follows:
>
>  >if (stream_id != program_stream_map
>  > && stream_id != padding_stream
>  > && 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) {
>
> So, private_stream_2 can not insert PTS/DTS. In addition, "ARIB-STD-B24,
> VOLUME3, Chapter5" defines private_stream_1 as "Synchronized PES"
> (mainly used for subtitles, with PTS/DTS inserted) and
> private_stream_2 as "Asynchronous PES" (mainly used for superinpose, no
> PTS/DTS inserted).

I'm working on ARIB related patches for mpegtsenc.c and later
I would like to provide a patch to fix the bug of private_stream_2.

In current situation, mpegts_write_pes() incorrectly writes
private_stream_2 with actually a private_stream_1-like PES header
that shouldn't appear according to the ISO/IEC 13818-1 standard.

Regards,
zheng

>  From these things, I misunderstood like that " PTS/DTS is generally
> inserted for private_stream_1, so the `if (stream_id ==
> STREAM_ID_PRIVATE_STREAM_1)` statement (I patched) must be a mistake! " ...
>
> Anyway, as I read the related code, I now understand that the problem
> should be solved specifically for ARIB-based transport stream (at
> "mpegts.c" or so.).
> Thank you again.
> _______________________________________________
> 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".
Mao Hata April 19, 2021, 5:25 p.m. UTC | #4 
On 2021/04/20 1:36, zheng qian wrote:
> I'm working on ARIB related patches for mpegtsenc.c and later
> I would like to provide a patch to fix the bug of private_stream_2.
> 
> In current situation, mpegts_write_pes() incorrectly writes
> private_stream_2 with actually a private_stream_1-like PES header
> that shouldn't appear according to the ISO/IEC 13818-1 standard.
> 
> Regards,
> zheng
> 

Oh, thank you! (and I should have searched the mailing list carefully 
before submitting my patch..)
Your posted patches might be close to what I'm wishing for. I will check 
them.
diff mbox series

Patch

diff --git a/libavformat/mpegts.h b/libavformat/mpegts.h
index 04874e0f42..ed640733e3 100644
--- a/libavformat/mpegts.h
+++ b/libavformat/mpegts.h
@@ -139,6 +139,7 @@ 
 
 /* ISO/IEC 13818-1 Table 2-22 */
 #define STREAM_ID_PRIVATE_STREAM_1   0xbd
+#define STREAM_ID_PRIVATE_STREAM_2   0xbf
 #define STREAM_ID_AUDIO_STREAM_0     0xc0
 #define STREAM_ID_VIDEO_STREAM_0     0xe0
 #define STREAM_ID_METADATA_STREAM    0xfc
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c
index a357f3a6aa..9982089d0f 100644
--- a/libavformat/mpegtsenc.c
+++ b/libavformat/mpegtsenc.c
@@ -1434,7 +1434,7 @@  static void mpegts_write_pes(AVFormatContext *s, AVStream *st,
             } else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
                 *q++ = stream_id != -1 ? stream_id : STREAM_ID_METADATA_STREAM;
 
-                if (stream_id == STREAM_ID_PRIVATE_STREAM_1) /* asynchronous KLV */
+                if (stream_id == STREAM_ID_PRIVATE_STREAM_2) /* asynchronous KLV */
                     pts = dts = AV_NOPTS_VALUE;
             } else {
                 *q++ = STREAM_ID_PRIVATE_STREAM_1;