From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id B20BC4B58A for ; Mon, 8 Jul 2024 22:07:10 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 57B3268DA2B; Tue, 9 Jul 2024 01:07:07 +0300 (EEST) Received: from mail-oa1-f41.google.com (mail-oa1-f41.google.com [209.85.160.41]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 70CCA68D90D for ; Tue, 9 Jul 2024 01:07:01 +0300 (EEST) Received: by mail-oa1-f41.google.com with SMTP id 586e51a60fabf-25982aa59efso2217426fac.3 for ; Mon, 08 Jul 2024 15:07:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1720476419; x=1721081219; darn=ffmpeg.org; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=tJ1gCDXu24YiPoMDNaBUn/hrEF8KPcYDReb6Rc1Cotk=; b=a4rjBlleL7E8mHG49tqeSnVvVgiN6NikpHEiXAThHKL4cgSkb1BJZhSFh+fzghmybE q6RRqb3rvYMZr0PM5d7FXB2T2XR9s0XxgyEBj5zDK7TI61U2qjbUz5EZXuBHM7pati3x ehUG1HMCfdpAW0A2xxlFa60WWYQNlQCMPyWNRafBfxxBhPf+6ZPPgOwDN7JkSNYFxi2h GcX7W+/yqOY+O6oYOHxq1slJ7604UP/9b5Opx+mD8J5OmaFtR1cmaBc1MMKNQNKP7Ve8 ezsZH4UqJXvczeN5hMzZThvkI386Q4M+hm5aJuqUDdkYJX2hCqmgqEOzYW/NEkx7ISWc UKUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720476419; x=1721081219; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tJ1gCDXu24YiPoMDNaBUn/hrEF8KPcYDReb6Rc1Cotk=; b=Uk5+2onrbkjdF1RSdPzxMe5Ds+0lvKsZWbfXDJ4kiJuh5iQiWfjTGucDApVKRSYjQn dcOwe2BCOAZU0OUlHv1id07aiJ9g8m0Qd+1RXExEkvVSHJfE2axsE4mPEdG1TruEd9Mc 9SicXUVPFKdUwPoBHXC3G3FRzblDeHPTFViXAwFHhwcP6YugIM30fV6y1K3+/jKlRQvQ eDiH0UjiMNsRKX3l4H291j5X48NgGUT/i/LnhJ5DAZoIojwczYSeeo+sjGl/93H7LkMD /r8FDmOZc4kuKVnAvl0wjgmc1gJFHPtIY4gdfFtfm3RWFGii24qojOgZGi9EbkbkAngD Yd+g== X-Gm-Message-State: AOJu0Yydyqtm2G4x/AOSx+hbDYj9Vo6r1yhSyU1I8JS82Ooo3Vxc0EWP MdONj3fh7tGwu6bVWRDWNu60gzp1N9E4QqPvw+Qp/lsQ52fTzbhTkV/FFSTKslhRP3KRhByNfFO EIsTvIKVL6dJ8zwUdfjy5/zNY/hJoolT+ X-Google-Smtp-Source: AGHT+IFfS6tW+c343zGkg5njLgg9WvvGDmnYtegVluOJP3uNJy1CSEGXkXmpAERWx5JLgTo1e7eWl1wQY1TXpEUZV+E= X-Received: by 2002:a05:6870:d892:b0:25e:14f0:62c4 with SMTP id 586e51a60fabf-25eae7f1ea4mr527139fac.18.1720476419169; Mon, 08 Jul 2024 15:06:59 -0700 (PDT) MIME-Version: 1.0 References: <20240703210506.75963-1-joshua.allmann@gmail.com> <20240706163710.GF4991@pb2> In-Reply-To: <20240706163710.GF4991@pb2> From: Josh Allmann Date: Mon, 8 Jul 2024 15:06:22 -0700 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH] avcodec/h264_mp4toannexb: Prepend SPS/PPS to buffering period SEI X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Sat, 6 Jul 2024 at 09:37, Michael Niedermayer 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 _______________________________________________ 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".