On Thu, 28. Nov 15:28, Andreas Rheinhardt wrote: > Andriy Gelman: > > On Tue, 26. Nov 07:24, Andriy Gelman wrote: > >> On Tue, 26. Nov 10:52, Michael Niedermayer wrote: > >>> On Mon, Nov 25, 2019 at 09:35:04PM -0500, Andriy Gelman wrote: > >>>> On Mon, 25. Nov 01:50, Michael Niedermayer wrote: > >>>>> On Sun, Nov 24, 2019 at 11:29:18AM -0500, Andriy Gelman wrote: > >>>>>> On Sun, 24. Nov 00:02, Michael Niedermayer wrote: > >>>>>>> On Tue, Oct 15, 2019 at 10:50:39PM -0400, Andriy Gelman wrote: > >>>>>>>> From: Andriy Gelman <andriy.gel...@gmail.com> > >>>>>>>> > >>>>>>>> Fixes #7799 > >>>>>>>> > >>>>>>>> Currently, the mp4toannexb filter always inserts the same extradata > >>>>>>>> at > >>>>>>>> the start of the first IRAP unit. As in ticket #7799, this can lead > >>>>>>>> to > >>>>>>>> decoding errors if modified parameter sets are signalled in-band. > >>>>>>>> > >>>>>>>> Decoding errors/visual artifacts are also present in the following > >>>>>>>> fate-suite/hevc-conformance datasets for hevc->mp4->hevc conversion: > >>>>>>>> -RAP_B_Bossen_1.bit > >>>>>>>> -RPS_C_ericsson_5.bit > >>>>>>>> -SLIST_A_Sony_4.bit > >>>>>>>> -SLIST_B_Sony_8.bit > >>>>>>>> -SLIST_C_Sony_3.bit > >>>>>>>> -SLIST_D_Sony_9.bit > >>>>>>>> -TSKIP_A_MS_2.bit > >>>>>>>> > >>>>>>>> This commit solves these errors by keeping track of VPS/SPS/PPS > >>>>>>>> parameter sets > >>>>>>>> during the conversion. The correct combination is inserted at the > >>>>>>>> start > >>>>>>>> of the first IRAP. SEIs from extradata are inserted before each IRAP. > >>>>>>>> > >>>>>>>> This commit also makes an update to the hevc-bsf-mp4toannexb fate > >>>>>>>> test > >>>>>>>> since the result before this patch contained duplicate parameter sets > >>>>>>>> in-band. > >>>>>>>> --- > >>>>>>>> libavcodec/hevc_mp4toannexb_bsf.c | 503 > >>>>>>>> ++++++++++++++++++++++++++++-- > >>>>>>>> tests/fate/hevc.mak | 2 +- > >>>>>>>> 2 files changed, 472 insertions(+), 33 deletions(-) > >>>>> [...] > >>>>>>> > >>>>>>> [...] > >>>>>>>> +/* > >>>>>>>> + * Function converts mp4 access unit into annexb > >>>>>>>> + * Output packet structure > >>>>>>>> + * VPS, SPS, PPS, [SEI(from extradata)], [SEI_PREFIX(from access > >>>>>>>> unit)], IRAP, [SEI_SUFFIX] > >>>>>>>> + * or > >>>>>>>> + * [SEI_PREFIX (from access unit)], [PPS (if not already > >>>>>>>> signalled)], VCL(non-irap), [SEI_SUFFIX] > >>>>>>>> + */ > >>>>>>>> static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out) > >>>>>>>> { > >>>>>>>> HEVCBSFContext *s = ctx->priv_data; > >>>>>>>> AVPacket *in; > >>>>>>>> - GetByteContext gb; > >>>>>>>> - > >>>>>>>> - int got_irap = 0; > >>>>>>>> - int i, ret = 0; > >>>>>>>> + H2645Packet pkt; > >>>>>>>> + int i, j, prev_size, ret; > >>>>>>>> + int irap_done; > >>>>>>>> > >>>>>>>> ret = ff_bsf_get_packet(ctx, &in); > >>>>>>>> if (ret < 0) > >>>>>>>> return ret; > >>>>>>>> > >>>>>>>> + /* output the annexb nalu if extradata is not parsed*/ > >>>>>>>> if (!s->extradata_parsed) { > >>>>>>>> av_packet_move_ref(out, in); > >>>>>>>> av_packet_free(&in); > >>>>>>>> return 0; > >>>>>>>> } > >>>>>>>> > >>>>>>> > >>>>>>>> - bytestream2_init(&gb, in->data, in->size); > >>>>>>> > >>>>>>> Is this really better without using an existing bytestream* API ? > >>>>>> > >>>>>> If I use the API, then I'd have to call bytestraem2_init for each nal > >>>>>> unit. I > >>>>>> also don't use the bytestream2_get_byte function. It seemed simpler to > >>>>>> use the WRITE_NAL macro. > >>>>>> > >>>>>> But maybe I've misunderstood your question. > >>>>> > >>>>> i had nothing really specific in mind, just the feeling that using none > >>>>> of > >>>>> the existing APIs there is a bit odd. > >>>>> > >>>>> but more specifically, what about the write side ? > >>>> > >>>> If I use the bytestream* API, then I'd have to add extra boiler plate > >>>> code each time I resize the output with av_grow_packet(). I'd have to > >>>> add something like: > >>>> > >>>> prev_size = bytestream2_tell_p(...); > >>>> bytestream2_init_writer(...); > >>>> bytestream2_skip_p(..., prev_size); > >>>> > >>>> Or maybe the API needs an extra function that reinitializes the pointers > >>>> but > >>>> keeps the current state of the writer. > >>> > >>> grow/realloc seems suboptimal to me for any API. > >>> cant you find out how much space is needed and allocate/grow just once > >>> then > >>> init the bytestream2 on that ? > >>> > >> > >> ok, I'll do it this way. > >> > > > > After spending so much time on this patch I've just discovered that the > > hevc_metadata bsf can do the same job as hevc_mp4toannexb. The former also > > inserts correct parameter sets and so fixes #7799... :( > > > > So instead of: > > ./ffmpeg -i in.mp4 -bsf:v hevc_mp4toannexb -codec:v copy out.hevc > > > > The user can just run: > > ./ffmpeg -i in.mp4 -bsf:v hevc_metadata -codec:v copy out.hevc > > > > One difference is that hevc_metadata currently only keeps the base layers > > (nuh_layer_id == 0), whereas hevc_mp4toannexb copies everything (before my > > patch). hevc_metadata will have a slightly higher complexity as it parses > > the full > > parameter sets. > > > > Can you give me advice on best way to proceed? > > > > If the nuh_layer_id issue is solved, can we remove the hevc_mp4toannexb > > filter > > and automatically insert hevc_metadata filter if the former is selected by > > the user? > > > Hello Andriy, > > 1. hevc_metadata inserts parameter sets? That is absolutely new to me. > How did you check this? (It does not insert parameter sets, but it > outputs annex b, so the sample from #7799 is left alone by > hevc_mp4toannexb after hevc_metadata, hence the updated in-band > extradata is not masked by non-updated header parameter sets).
You are right, it doesn't insert the referenced parameter sets, but just doesn't overwrite them by the inserting the old extradata. I suppose if the media is spliced, then my patch would be still be better than hevc_metadata because all the parameter are guaranteed to be in the cvs. But there's still overlap imo. > 2. hevc_metadata is based upon cbs and cbs for H.264 and H.265 uses > the h2645_parse functions and the nuh_layer_id issue affects it, too. > 3. cbs is (currently) slow. That is because it lets the h2645_parse > functions unescape the whole units and the reassembles them at the > end. Even though I have an idea for a patch that would greatly speed > this up, it will never be as fast as an approach that does not rely on > cbs. I'm also using ff_h2645_packet_split in my patch, too. It gives the rbsp on all the NALs. I think it's possible to set some kind of a bound where only the first x bytes are needed. > 4. Furthermore, cbs is very picky and drops whole packets because of > minor errors. Not what you would want in an auto-inserted bsf. I've only glanced over cbs so can't really comment. Thanks, -- Andriy _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".