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