Re: [FFmpeg-devel] [PATCH v5 04/10] avcodec: add MP4 to annexb support for H266/VVC

[Thomas Siedel <thomas.ff@spin-digital.com>]

On Wed, 1 Feb 2023 at 16:34, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:

>
>
> > On Jan 3, 2023, at 21:40, Thomas Siedel <thomas.ff@spin-digital.com>
> wrote:
> >
> > Add parser for VVC MP4 to Annex B byte stream format.
> >
> > Co-authored-by: Nuo Mi <nuomi2021@gmail.com>
> > ---
> > configure                        |   1 +
> > libavcodec/Makefile              |   2 +
> > libavcodec/bitstream_filters.c   |   2 +
> > libavcodec/h266_metadata_bsf.c   | 146 ++++++++++++++
> > libavcodec/vvc_mp4toannexb_bsf.c | 329 +++++++++++++++++++++++++++++++
>
> Please add these bsfs in separate patches.
>

Thank you for your feedback.
I separated them now in the new patch set version 6.


> > 5 files changed, 480 insertions(+)
> > create mode 100644 libavcodec/h266_metadata_bsf.c
> > create mode 100644 libavcodec/vvc_mp4toannexb_bsf.c
>
> I don’t know the reason behind h265_metadata_bsf/hevc_mp4toannexb_bsf,
> but I prefer use the same prefix unless there are technical reasons.
> Such alias is annoying when browsing and searching the source code.
>

I took the naming of the HEVC implementation as an orientation.
But yes, I agree that this kind of mix might be problematic.
In the new patch set, I now decided on the ITU syntax and renamed all the
files and functions from vvc to h266 prefix.
With this, they are now also alphabetically close to h264 and hevc.
However, I did not rename the VVC_* enum types, because this would change a
lot of code, and I am worried that
this makes things more complicated when comparing different versions of the
code, e.g., for review.

What do you, and the other developers, think about this?
Which naming pattern would you prefer? Should the enums also be renamed?


> > --- a/libavcodec/bitstream_filters.c
> > +++ b/libavcodec/bitstream_filters.c
> > @@ -64,6 +64,8 @@ extern const FFBitStreamFilter ff_vp9_metadata_bsf;
> > extern const FFBitStreamFilter ff_vp9_raw_reorder_bsf;
> > extern const FFBitStreamFilter ff_vp9_superframe_bsf;
> > extern const FFBitStreamFilter ff_vp9_superframe_split_bsf;
> > +extern const FFBitStreamFilter ff_vvc_mp4toannexb_bsf;
> > +extern const FFBitStreamFilter ff_vvc_metadata_bsf;
>
> Please sort by alphabetical order.
>

OK, this is done now in the new patch set version.


>
> >
> > #include "libavcodec/bsf_list.c"
> >
> > diff --git a/libavcodec/h266_metadata_bsf.c
> b/libavcodec/h266_metadata_bsf.c
> > new file mode 100644
> > index 0000000000..f2bd2f31f3
> > --- /dev/null
> > +++ b/libavcodec/h266_metadata_bsf.c
> > @@ -0,0 +1,146 @@
> > +/*
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> > + */
> > +
> > +#include "libavutil/common.h"
> > +#include "libavutil/opt.h"
> > +
> > +#include "bsf.h"
> > +#include "bsf_internal.h"
> > +#include "cbs.h"
> > +#include "cbs_bsf.h"
> > +#include "cbs_h266.h"
> > +#include "vvc.h"
> > +
> > +#define IS_SLICE(nut) (nut <= VVC_RASL_NUT || (nut >= VVC_IDR_W_RADL &&
> nut <= VVC_GDR_NUT))
> > +#define IS_PH(nut) (nut == VVC_PH_NUT)
>
> They are duplicated inside vvc_parser.c. How about add a prefix and share
> these macros?
>

Thank you for the suggestion.
I did it now in the new patch set version.


>
> > +
> > +typedef struct VVCMetadataContext {
> > +    CBSBSFContext common;
> > +
> > +    H266RawAUD aud_nal;
> > +
> > +    int aud;
> > +} VVCMetadataContext;
> > +
> > +static int h266_metadata_update_fragment(AVBSFContext *bsf, AVPacket
> *pkt,
> > +                                         CodedBitstreamFragment *pu)
> > +{
> > +    VVCMetadataContext *ctx = bsf->priv_data;
> > +    int err, i;
> > +
> > +    // If an AUD is present, it must be the first NAL unit.
> > +    if (pu->units[0].type == VVC_AUD_NUT) {
> > +        if (ctx->aud == BSF_ELEMENT_REMOVE)
> > +            ff_cbs_delete_unit(pu, 0);
> > +    } else {
> > +        if (ctx->aud == BSF_ELEMENT_INSERT) {
>
> Should check pkt != NULL here.
>
> `else if` can save one level of indentation.
>

OK, this is done now in the new patch set version.


> > +
> > +const FFBitStreamFilter ff_vvc_metadata_bsf = {
> > +    .p.name         = "vvc_metadata",
> > +    .p.codec_ids    = vvc_metadata_codec_ids,
> > +    .p.priv_class   = &vvc_metadata_class,
> > +    .priv_data_size = sizeof(VVCMetadataContext),
> > +    .init           = &vvc_metadata_init,
> > +    .close          = &ff_cbs_bsf_generic_close,
> > +    .filter         = &ff_cbs_bsf_generic_filter,
> > +};
>
> The function of vvc_metadata is very limited, I’d like to suggest
> add more features with v2. It can be done after other patches.
>
>
Indeed (currently only aud is implemented).
More metadata should be implemented after the first version is merged.
_______________________________________________
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".