Re: [FFmpeg-devel] [PATCH v5 04/10] avcodec: add MP4 to annexb support for H266/VVC
On Wed, 1 Feb 2023 at 16:34, zhilizhao(赵志立) wrote:
>
>
> > On Jan 3, 2023, at 21:40, Thomas Siedel
> wrote:
> >
> > Add parser for VVC MP4 to Annex B byte stream format.
> >
> > Co-authored-by: Nuo Mi
> > ---
> > 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 00..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 = _metadata_class,
> > +.priv_data_size = sizeof(VVCMetadataContext),
> > +.init = _metadata_init,
> > +.close
Re: [FFmpeg-devel] [PATCH v5 04/10] avcodec: add MP4 to annexb support for H266/VVC
> On Jan 3, 2023, at 21:40, Thomas Siedel wrote:
>
> Add parser for VVC MP4 to Annex B byte stream format.
>
> Co-authored-by: Nuo Mi
> ---
> 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.
> 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.
>
> diff --git a/configure b/configure
> index 2408dca0f5..776a972663 100755
> --- a/configure
> +++ b/configure
> @@ -3286,6 +3286,7 @@ mjpeg2jpeg_bsf_select="jpegtables"
> mpeg2_metadata_bsf_select="cbs_mpeg2"
> trace_headers_bsf_select="cbs"
> vp9_metadata_bsf_select="cbs_vp9"
> +vvc_metadata_bsf_select="cbs_h266"
>
> # external libraries
> aac_at_decoder_deps="audiotoolbox"
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 3e858b200b..2dee099f25 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -1236,6 +1236,8 @@ OBJS-$(CONFIG_VP9_METADATA_BSF) +=
> vp9_metadata_bsf.o
> OBJS-$(CONFIG_VP9_RAW_REORDER_BSF)+= vp9_raw_reorder_bsf.o
> OBJS-$(CONFIG_VP9_SUPERFRAME_BSF) += vp9_superframe_bsf.o
> OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF) += vp9_superframe_split_bsf.o
> +OBJS-$(CONFIG_VVC_METADATA_BSF) += h266_metadata_bsf.o
> +OBJS-$(CONFIG_VVC_MP4TOANNEXB_BSF)+= vvc_mp4toannexb_bsf.o
>
> # thread libraries
> OBJS-$(HAVE_LIBC_MSVCRT) += file_open.o
> diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
> index a3bebefe5f..403884f3d7 100644
> --- 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.
>
> #include "libavcodec/bsf_list.c"
>
> diff --git a/libavcodec/h266_metadata_bsf.c b/libavcodec/h266_metadata_bsf.c
> new file mode 100644
> index 00..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?
> +
> +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.
> +const H266RawSlice *first_slice = NULL;
> +const H266RawPH *ph = NULL;
> +H266RawAUD *aud = >aud_nal;
> +int pic_type = 0, temporal_id = 8, layer_id = 0;
> +for (i = 0; i < pu->nb_units; i++) {
> +const H266RawNALUnitHeader *nal =
[FFmpeg-devel] [PATCH v5 04/10] avcodec: add MP4 to annexb support for H266/VVC
Add parser for VVC MP4 to Annex B byte stream format.
Co-authored-by: Nuo Mi
---
configure| 1 +
libavcodec/Makefile | 2 +
libavcodec/bitstream_filters.c | 2 +
libavcodec/h266_metadata_bsf.c | 146 ++
libavcodec/vvc_mp4toannexb_bsf.c | 329 +++
5 files changed, 480 insertions(+)
create mode 100644 libavcodec/h266_metadata_bsf.c
create mode 100644 libavcodec/vvc_mp4toannexb_bsf.c
diff --git a/configure b/configure
index 2408dca0f5..776a972663 100755
--- a/configure
+++ b/configure
@@ -3286,6 +3286,7 @@ mjpeg2jpeg_bsf_select="jpegtables"
mpeg2_metadata_bsf_select="cbs_mpeg2"
trace_headers_bsf_select="cbs"
vp9_metadata_bsf_select="cbs_vp9"
+vvc_metadata_bsf_select="cbs_h266"
# external libraries
aac_at_decoder_deps="audiotoolbox"
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3e858b200b..2dee099f25 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -1236,6 +1236,8 @@ OBJS-$(CONFIG_VP9_METADATA_BSF) +=
vp9_metadata_bsf.o
OBJS-$(CONFIG_VP9_RAW_REORDER_BSF)+= vp9_raw_reorder_bsf.o
OBJS-$(CONFIG_VP9_SUPERFRAME_BSF) += vp9_superframe_bsf.o
OBJS-$(CONFIG_VP9_SUPERFRAME_SPLIT_BSF) += vp9_superframe_split_bsf.o
+OBJS-$(CONFIG_VVC_METADATA_BSF) += h266_metadata_bsf.o
+OBJS-$(CONFIG_VVC_MP4TOANNEXB_BSF)+= vvc_mp4toannexb_bsf.o
# thread libraries
OBJS-$(HAVE_LIBC_MSVCRT) += file_open.o
diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c
index a3bebefe5f..403884f3d7 100644
--- 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;
#include "libavcodec/bsf_list.c"
diff --git a/libavcodec/h266_metadata_bsf.c b/libavcodec/h266_metadata_bsf.c
new file mode 100644
index 00..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)
+
+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) {
+const H266RawSlice *first_slice = NULL;
+const H266RawPH *ph = NULL;
+H266RawAUD *aud = >aud_nal;
+int pic_type = 0, temporal_id = 8, layer_id = 0;
+for (i = 0; i < pu->nb_units; i++) {
+const H266RawNALUnitHeader *nal = pu->units[i].content;
+if (!nal)
+continue;
+if (nal->nuh_temporal_id_plus1 < temporal_id + 1)
+temporal_id = nal->nuh_temporal_id_plus1 - 1;
+if (IS_PH(nal->nal_unit_type)) {
+ph = pu->units[i].content;
+} else if (IS_SLICE(nal->nal_unit_type)) {
+const H266RawSlice *slice = pu->units[i].content;
+layer_id = nal->nuh_layer_id;
+if (slice->header.sh_slice_type == VVC_SLICE_TYPE_B &&
+pic_type < 2)
+pic_type = 2;
+if (slice->header.sh_slice_type == VVC_SLICE_TYPE_P &&
+
