[FFmpeg-devel] [PATCH] h264_mp4toannexb_bsf: always set idr_sps_pps_seen when SPS/PPS is seen.
Michael Niedermayer
michaelni at gmx.at
Tue Sep 30 00:54:52 CEST 2014
On Mon, Sep 29, 2014 at 03:31:36PM +0200, Benoit Fouet wrote:
> Hi,
>
> ----- Mail original -----
> > Hi,
> >
> > Le 26/09/2014 18:38, Michael Niedermayer a écrit :
> > > On Fri, Sep 26, 2014 at 09:27:01AM +0200, Benoit Fouet wrote:
> > >> Hi,
> > >>
> > >> ----- Mail original -----
> > >>> Michael Niedermayer <michaelni <at> gmx.at> writes:
> > >>>
> > >>>>
> > >>>> On Fri, Aug 01, 2014 at 01:54:14AM +0200, Michael Niedermayer
> > >>>> wrote:
> > >>>>> On Thu, Jul 31, 2014 at 03:40:51PM +0200, Benoit Fouet wrote:
> > >>>>>> In order not to break a sequence like "SPS IDR SPS IDR", the
> > >>> boolean
> > >>>>>> telling that the SPS/PPS has been seen should always be set.
> > >>>>>> ---
> > >>>>>> libavcodec/h264_mp4toannexb_bsf.c | 2 +-
> > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>
> > >>>>> LGTM
> > >>>>
> > >>>> applied
> > >>>>
> > >>>> thanks
> > >>>
> > >>> A GitHub user "@thomag" commented on this commit:
> > >>>
> > >>>> Consider an mp4 files with just pps in the h264 stream (no sps
> > >>>> in
> > >>>> the
> > >>>> NAL units, but available from avcc).
> > >>>> In this case 'ctx->idr_sps_pps_seen' prevents inserting sps in
> > >>>> the
> > >>>> extracted h264, which is then unusable.
> > >>>
> > >>> FYI. I cannot confirm if it is correct or not, and what the
> > >>> correct
> > >>> solution would be.
> > >>>
> > >>
> > >> I'm willing to try and fix this when/if we have a sample to test
> > >> that.
> > >> If there is also an example with just sps in the avcc and only pps
> > >> in
> > the stream, that'd be perfect...
> > >
> > > forwarding from github
> > >
> > > Michael,
> > > Here is a short clip @
> > https://s3.amazonaws.com/quickfire-public/dwclip2.mp4
> > > which has just PPS in the h264 stream.
> > > Please let me know if you need anything else.
> > > Regards,
> > > Thomas.
> > >
> >
> > OK, cool. I'll have a look at this after the weekend then.
> >
>
> I have a first version of the patch attached.
> With this patch, the sample provided from the link above can be converted with the h264_mp4toannexb_bsf, but produces warnings when played back.
> What I've done here is to prepend only the missing parameter set to the filtered stream.
> I'll spend some more time on it, but I'd like to get another look than mine, just in case I'm missing something obvious.
>
> --
> Ben
> h264_mp4toannexb_bsf.c | 44 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 9 deletions(-)
> c52f2d890241776464be2e7fb3a0acc9bd0ecdfc 0001-avcodec-h264_mp4toannexb_bsf-add-a-case-when-only-SP.patch
> From 29536721f09901f0197c0ca4c14e8e19ff01489b Mon Sep 17 00:00:00 2001
> From: Benoit Fouet <benoit.fouet at free.fr>
> Date: Mon, 29 Sep 2014 15:18:50 +0200
> Subject: [PATCH] avcodec/h264_mp4toannexb_bsf: add a case when only SPS/PPS is
> in the stream.
>
> When the AVCC contains SPS and PPS and the stream contains only one of
> those, only the missing one should be prepended to the next key frame,
> not both.
> ---
> libavcodec/h264_mp4toannexb_bsf.c | 44 +++++++++++++++++++++++++++++++--------
> 1 file changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
> index 739ff95..6ab76ae 100644
> --- a/libavcodec/h264_mp4toannexb_bsf.c
> +++ b/libavcodec/h264_mp4toannexb_bsf.c
> @@ -26,9 +26,12 @@
> #include "avcodec.h"
>
> typedef struct H264BSFContext {
> + int32_t sps_offset;
> + int32_t pps_offset;
> uint8_t length_size;
> uint8_t new_idr;
> - uint8_t idr_sps_pps_seen;
> + uint8_t idr_sps_seen;
> + uint8_t idr_pps_seen;
> int extradata_parsed;
> } H264BSFContext;
>
> @@ -60,7 +63,7 @@ static int alloc_and_copy(uint8_t **poutbuf, int *poutbuf_size,
> return 0;
> }
>
> -static int h264_extradata_to_annexb(AVCodecContext *avctx, const int padding)
> +static int h264_extradata_to_annexb(H264BSFContext *ctx, AVCodecContext *avctx, const int padding)
> {
> uint16_t unit_size;
> uint64_t total_size = 0;
> @@ -70,11 +73,14 @@ static int h264_extradata_to_annexb(AVCodecContext *avctx, const int padding)
> static const uint8_t nalu_header[4] = { 0, 0, 0, 1 };
> int length_size = (*extradata++ & 0x3) + 1; // retrieve length coded size
>
> + ctx->sps_offset = ctx->pps_offset = -1;
> +
> /* retrieve sps and pps unit(s) */
> unit_nb = *extradata++ & 0x1f; /* number of sps unit(s) */
> if (!unit_nb) {
> goto pps;
> } else {
> + ctx->sps_offset = 0;
> sps_seen = 1;
> }
>
> @@ -103,8 +109,10 @@ static int h264_extradata_to_annexb(AVCodecContext *avctx, const int padding)
> pps:
> if (!unit_nb && !sps_done++) {
> unit_nb = *extradata++; /* number of pps unit(s) */
> - if (unit_nb)
> + if (unit_nb) {
> + ctx->pps_offset = (extradata - 1) - (avctx->extradata + 4);
> pps_seen = 1;
> + }
> }
> }
>
> @@ -151,12 +159,13 @@ static int h264_mp4toannexb_filter(AVBitStreamFilterContext *bsfc,
>
> /* retrieve sps and pps NAL units from extradata */
> if (!ctx->extradata_parsed) {
> - ret = h264_extradata_to_annexb(avctx, FF_INPUT_BUFFER_PADDING_SIZE);
> + ret = h264_extradata_to_annexb(ctx, avctx, FF_INPUT_BUFFER_PADDING_SIZE);
> if (ret < 0)
> return ret;
> ctx->length_size = ret;
> ctx->new_idr = 1;
> - ctx->idr_sps_pps_seen = 0;
> + ctx->idr_sps_seen = 0;
> + ctx->idr_pps_seen = 0;
> ctx->extradata_parsed = 1;
> }
>
> @@ -176,8 +185,10 @@ static int h264_mp4toannexb_filter(AVBitStreamFilterContext *bsfc,
> if (buf + nal_size > buf_end || nal_size < 0)
> goto fail;
>
> - if (unit_type == 7 || unit_type == 8)
> - ctx->idr_sps_pps_seen = 1;
> + if (unit_type == 7)
> + ctx->idr_sps_seen = 1;
> + else if (unit_type == 8)
> + ctx->idr_pps_seen = 1;
>
> /* if this is a new IDR picture following an IDR picture, reset the idr flag.
> * Just check first_mb_in_slice to be 0 as this is the simplest solution.
> @@ -186,19 +197,34 @@ static int h264_mp4toannexb_filter(AVBitStreamFilterContext *bsfc,
> ctx->new_idr = 1;
>
> /* prepend only to the first type 5 NAL unit of an IDR picture, if no sps/pps are already present */
> - if (ctx->new_idr && unit_type == 5 && !ctx->idr_sps_pps_seen) {
> + if (ctx->new_idr && unit_type == 5 && !ctx->idr_sps_seen && !ctx->idr_pps_seen) {
> if ((ret=alloc_and_copy(poutbuf, poutbuf_size,
> avctx->extradata, avctx->extradata_size,
> buf, nal_size)) < 0)
> goto fail;
> ctx->new_idr = 0;
> + /* if only sps or pps has been seen, prepend the other one */
> + } else if (ctx->new_idr && unit_type == 5 && (ctx->idr_sps_seen ^ ctx->idr_pps_seen)) {
> + int offset = ctx->idr_sps_seen ? ctx->pps_offset : ctx->sps_offset;
> + int size = ctx->idr_sps_seen ? avctx->extradata_size - ctx->pps_offset : ctx->pps_offset;
> + if (offset == -1) {
> + av_log(avctx, AV_LOG_WARNING, "%s not present in the stream, nor in AVCC, stream may be unreadable\n",
> + ctx->idr_pps_seen ? "SPS" : "PPS");
> + offset = 0;
> + size = avctx->extradata_size;
> + }
> + if ((ret = alloc_and_copy(poutbuf, poutbuf_size,
> + avctx->extradata + offset, size,
> + buf, nal_size)) < 0)
> + goto fail;
does this ensure that the sps is before the pps ?
if not that might be the reason for the warnings
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140930/ef6ad0de/attachment.asc>
More information about the ffmpeg-devel
mailing list