| Message ID | 20230131124512.281726-1-jpcoiner@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] libavformat/mpegtsenc.c -- correctly re-emit extradata ahead of IDR pictures | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, 31 Jan 2023, John Coiner wrote: > This replaces and obsoletes the earlier version of this patch, which had a bug. Thanks to Marton Balint for spotting it. > > This fixes https://trac.ffmpeg.org/ticket/10148, it ensures that each IDR frame is prefixed with SPS/PPS. > > Tested manually, with inputs containing AUDs (x264's aud=1 mode) and also without (aud=0) to confirm it still inserts AUDs when necessary, and not otherwise. Your code duplicates AUD-s if they are already present and sps is also added: if original packet had [AUD][IDR] it will become [AUD][SPS][PPS][AUD][IDR]. Regards, Marton > > --- > libavformat/mpegtsenc.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c > index 48d39e6a7d..64948f4d75 100644 > --- a/libavformat/mpegtsenc.c > +++ b/libavformat/mpegtsenc.c > @@ -1877,6 +1877,7 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > > if (st->codecpar->codec_id == AV_CODEC_ID_H264) { > const uint8_t *p = buf, *buf_end = p + size; > + uint8_t found_aud = 0; > uint32_t state = -1; > int extradd = (pkt->flags & AV_PKT_FLAG_KEY) ? st->codecpar->extradata_size : 0; > int ret = ff_check_h264_startcode(s, st, pkt); > @@ -1886,17 +1887,25 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) > if (extradd && AV_RB24(st->codecpar->extradata) > 1) > extradd = 0; > > + // Ensure that all pictures are prefixed with an AUD, and that > + // IDR pictures are also prefixed with SPS and PPS. SPS and PPS > + // are assumed to be available in 'extradata' if not found in-band. > do { > p = avpriv_find_start_code(p, buf_end, &state); > av_log(s, AV_LOG_TRACE, "nal %"PRId32"\n", state & 0x1f); > - if ((state & 0x1f) == 7) > + if ((state & 0x1f) == 7) // SPS NAL > extradd = 0; > - } while (p < buf_end && (state & 0x1f) != 9 && > - (state & 0x1f) != 5 && (state & 0x1f) != 1); > - > - if ((state & 0x1f) != 5) > + if ((state & 0x1f) == 9) // AUD > + found_aud = 1; > + } while (p < buf_end > + && (state & 0x1f) != 5 // IDR picture > + && (state & 0x1f) != 1 // non-IDR picture > + && (extradd > 0 || !found_aud)); > + if ((state & 0x1f) != 5) { > + // Did not find IDR picture; do not emit extradata. > extradd = 0; > - if ((state & 0x1f) != 9) { // AUD NAL > + } > + if (extradd > 0 || !found_aud) { > data = av_malloc(pkt->size + 6 + extradd); > if (!data) > return AVERROR(ENOMEM); > -- > 2.39.1.456.gfc5497dd1b-goog > > _______________________________________________ > 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". >
On Sun, Feb 5, 2023 at 6:33 PM Marton Balint <cus@passwd.hu> wrote: > Your code duplicates AUD-s if they are already present and sps is also > added: > > if original packet had > [AUD][IDR] > it will become > [AUD][SPS][PPS][AUD][IDR]. > > Regards, > Marton Hi Marton, You're right, this is not compliant. The H.264 spec (section 7.4.1.2.3) is clear about this, it says: - There should be at most one AUD per access unit - Each access unit should include a picture. (We can't produce a fake "access unit" to carry the SPS and PPS alone; that's not allowed.) - When AUD is present, it should be the first NAL in the access unit. (We can't just prepend SPS and PPS ahead of the AUD.) Perhaps you knew all this; I am just learning. :-) So if the original packet has [AUD][IDR], then we should produce [AUD][SPS][PPS][IDR]. And if the original packet has [SEI][AUD][IDR], then we should produce [AUD][SPS][PPS][SEI][IDR] -- chopping out the original AUD, and then inserting the prefix with the new AUD. Tests show x264 producing the [SEI][AUD][IDR] sequence, despite it being contrary to the spec's recommendation that AUD should be first. Does that seem like the correct handling? If so I'll post a v2 patch for it. Thanks again. John
diff --git a/libavformat/mpegtsenc.c b/libavformat/mpegtsenc.c index 48d39e6a7d..64948f4d75 100644 --- a/libavformat/mpegtsenc.c +++ b/libavformat/mpegtsenc.c @@ -1877,6 +1877,7 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) if (st->codecpar->codec_id == AV_CODEC_ID_H264) { const uint8_t *p = buf, *buf_end = p + size; + uint8_t found_aud = 0; uint32_t state = -1; int extradd = (pkt->flags & AV_PKT_FLAG_KEY) ? st->codecpar->extradata_size : 0; int ret = ff_check_h264_startcode(s, st, pkt); @@ -1886,17 +1887,25 @@ static int mpegts_write_packet_internal(AVFormatContext *s, AVPacket *pkt) if (extradd && AV_RB24(st->codecpar->extradata) > 1) extradd = 0; + // Ensure that all pictures are prefixed with an AUD, and that + // IDR pictures are also prefixed with SPS and PPS. SPS and PPS + // are assumed to be available in 'extradata' if not found in-band. do { p = avpriv_find_start_code(p, buf_end, &state); av_log(s, AV_LOG_TRACE, "nal %"PRId32"\n", state & 0x1f); - if ((state & 0x1f) == 7) + if ((state & 0x1f) == 7) // SPS NAL extradd = 0; - } while (p < buf_end && (state & 0x1f) != 9 && - (state & 0x1f) != 5 && (state & 0x1f) != 1); - - if ((state & 0x1f) != 5) + if ((state & 0x1f) == 9) // AUD + found_aud = 1; + } while (p < buf_end + && (state & 0x1f) != 5 // IDR picture + && (state & 0x1f) != 1 // non-IDR picture + && (extradd > 0 || !found_aud)); + if ((state & 0x1f) != 5) { + // Did not find IDR picture; do not emit extradata. extradd = 0; - if ((state & 0x1f) != 9) { // AUD NAL + } + if (extradd > 0 || !found_aud) { data = av_malloc(pkt->size + 6 + extradd); if (!data) return AVERROR(ENOMEM);