| Message ID | 20240703210506.75963-1-joshua.allmann@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On Wed, Jul 03, 2024 at 02:05:06PM -0700, Josh Allmann wrote: > Encoders may emit a buffering period SEI without a corresponding > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > During Annex B conversion, this may result in the SPS/PPS being > inserted *after* the buffering period SEI but before the IDR NAL. > > Since the buffering period SEI references the SPS, the SPS/PPS > needs to come first. > --- > libavcodec/bsf/h264_mp4toannexb.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) breaks fate TEST h264-bsf-mp4toannexb --- ./tests/ref/fate/h264-bsf-mp4toannexb 2024-07-01 23:30:40.656213791 +0200 +++ tests/data/fate/h264-bsf-mp4toannexb 2024-07-06 12:13:56.491072296 +0200 @@ -1 +1 @@ -5f04c27cc6ee8625fe2405fb0f7da9a3 +ff2551123909f54c382294baa1bb4364 Test h264-bsf-mp4toannexb failed. Look at tests/data/fate/h264-bsf-mp4toannexb.err for details. make: *** [tests/Makefile:311: fate-h264-bsf-mp4toannexb] Error 1 [...]
On Sat, 6 Jul 2024 at 09:37, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Wed, Jul 03, 2024 at 02:05:06PM -0700, Josh Allmann wrote: > > Encoders may emit a buffering period SEI without a corresponding > > SPS/PPS if the SPS/PPS is carried out-of-band, eg with avcc. > > > > During Annex B conversion, this may result in the SPS/PPS being > > inserted *after* the buffering period SEI but before the IDR NAL. > > > > Since the buffering period SEI references the SPS, the SPS/PPS > > needs to come first. > > --- > > libavcodec/bsf/h264_mp4toannexb.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > breaks fate > > TEST h264-bsf-mp4toannexb > --- ./tests/ref/fate/h264-bsf-mp4toannexb 2024-07-01 23:30:40.656213791 +0200 > +++ tests/data/fate/h264-bsf-mp4toannexb 2024-07-06 12:13:56.491072296 +0200 > @@ -1 +1 @@ > -5f04c27cc6ee8625fe2405fb0f7da9a3 > +ff2551123909f54c382294baa1bb4364 > Test h264-bsf-mp4toannexb failed. Look at tests/data/fate/h264-bsf-mp4toannexb.err for details. > make: *** [tests/Makefile:311: fate-h264-bsf-mp4toannexb] Error 1 > Thanks for the heads up. Took a look into each of the failing fate tests from [0] and I think these changes are expected and OK. Each of the failing tests shows the problem that this patch fixes, which is that the SPS/PPS is written after the buffering period SEI. An easy way to eyeball the issue is that probing the Annex B output logs an error which says "non-existing SPS 0 referenced in buffering period" - in fact this is why I wrote this patch, to get to the bottom of that message. The NAL ordering can also be inspected with `bsf:v trace_headers`. There also seems to be a side benefit that makes segmenting more robust - details below. Some notes on each failing test: fate-segment-mp4-to-ts : Before this patch, the segments produced after 000.ts are not independently decodable, because only the first segment has any extradata [1]. After the patch, the segments can be decoded independently. Unless the intent of the test is to actually produce broken segments, this patch is probably correct in fixing that. Also see the `fate-h264-bsf-mp4toannexb` testcase. fate-h264-bsf-mp4toannexb : In the original version, the SPS/PPS is only written once - maybe because there are no true IDRs after the first frame, only recovery point SEIs. In the patched version, the SPS/PPS is written before each buffering period SEI, 6 times in total. I can see how this might be strictly unnecessary, but probably harmless from a spec standpoint. Also it seems to make segmented muxing more robust, since this testcase shares the same input as `fate-segment-mp4-to-ts` which produces broken segments without the patch. fate-h264_mp4toannexb_ticket2991 : This is a clean example of the problem that this patch solves: without it, a buffering period SEI comes before the SPS/PPS. Inspecting a diff of the NAL units [2], the only change is in the reordering of the NALs such that the SPS/PPS comes before the buffering period SEI, rather than after. If all that seems OK, then I can send another patch to update the fate references to match the new values. [0] https://patchwork.ffmpeg.org/check/104951/ [1] The first segment has extradata, but it is still in the wrong order without the patch. [2] https://gist.github.com/j0sh/c912056138822c4d8c9564f4062e1e7b Josh
diff --git a/libavcodec/bsf/h264_mp4toannexb.c b/libavcodec/bsf/h264_mp4toannexb.c index 92af6a6881..6607f1e91a 100644 --- a/libavcodec/bsf/h264_mp4toannexb.c +++ b/libavcodec/bsf/h264_mp4toannexb.c @@ -363,6 +363,19 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt) if (!new_idr && unit_type == H264_NAL_IDR_SLICE && (buf[1] & 0x80)) new_idr = 1; + /* If this is a buffering period SEI without a corresponding sps/pps + * then prepend any existing sps/pps before the SEI */ + if (unit_type == H264_NAL_SEI && buf[1] == 0 && !sps_seen && !pps_seen) { + if (s->sps_size) { + count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j); + sps_seen = 1; + } + if (s->pps_size) { + count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j); + pps_seen = 1; + } + } + /* prepend only to the first type 5 NAL unit of an IDR picture, if no sps/pps are already present */ if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) { if (s->sps_size)