Re: [FFmpeg-devel] [PATCH v5 1/6] lavu/frame: Add Dolby Vision metadata side data type

Re: [FFmpeg-devel] [PATCH v5 1/6] lavu/frame: Add Dolby Vision metadata side data type

[Derek Buitenhuis]

On 12/15/2021 11:33 AM, Hendrik Leppkes wrote:
> Any more comments on this set? LGTM now, but others have also had
> comments before.
> I would prefer if this gets in soon before 5.0, so user-apps can start
> making use of this data.

No objections from me.

- Derek
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 1/6] lavu/frame: Add Dolby Vision metadata side data type

[Hendrik Leppkes]

On Wed, Dec 15, 2021 at 12:33 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Sun, Dec 12, 2021 at 12:56 PM Niklas Haas <ffmpeg@haasn.xyz> wrote:
> >
> > From: Niklas Haas <git@haasn.dev>
> >
>
> Any more comments on this set? LGTM now, but others have also had
> comments before.
> I would prefer if this gets in soon before 5.0, so user-apps can start
> making use of this data.
>

If nothing else comes up i'll apply this set after the weekend.

- Hendrik
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 1/6] lavu/frame: Add Dolby Vision metadata side data type

[Anton Khirnov]

Quoting Niklas Haas (2021-12-12 12:55:41)
> +/**
> + * Coefficients of a piece-wise function. The pieces of the function span the
> + * value ranges between two adjacent pivot values.
> + */
> +#define FF_DOVI_MAX_PIECES 8

Why FF_? It's public, no?

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 1/6] lavu/frame: Add Dolby Vision metadata side data type

[Anton Khirnov]

Quoting Niklas Haas (2021-12-12 12:55:41)
> From: Niklas Haas <git@haasn.dev>
> 
> Signed-off-by: Niklas Haas <git@haasn.dev>
> ---
>  doc/APIchanges        |   3 +
>  libavutil/dovi_meta.c |  12 ++++
>  libavutil/dovi_meta.h | 132 ++++++++++++++++++++++++++++++++++++++++++
>  libavutil/frame.c     |   1 +
>  libavutil/frame.h     |   9 ++-
>  libavutil/version.h   |   2 +-
>  6 files changed, 157 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 17aa664ca3..0ddde40bdf 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
>  
>  API changes, most recent first:
>  
> +2021-12-xx - xxxxxxxxxx - lavu 57.12.100 - frame.h
> +  Add AV_FRAME_DATA_DOVI_RESHAPING.

Doesn't match the actual name.

> +
>  2021-12-xx - xxxxxxxxxx - lavf 59.10.100 - avformat.h
>    Add AVFormatContext io_close2 which returns an int
>  
> diff --git a/libavutil/dovi_meta.c b/libavutil/dovi_meta.c
> index 7bd08f6c54..60b4cb2376 100644
> --- a/libavutil/dovi_meta.c
> +++ b/libavutil/dovi_meta.c
> @@ -33,3 +33,15 @@ AVDOVIDecoderConfigurationRecord *av_dovi_alloc(size_t *size)
>  
>      return dovi;
>  }
> +
> +AVDOVIMetadata *av_dovi_metadata_alloc(size_t *size)
> +{
> +    AVDOVIMetadata *dovi = av_mallocz(sizeof(AVDOVIMetadata));
> +    if (!dovi)
> +        return NULL;
> +
> +    if (size)
> +        *size = sizeof(*dovi);
> +
> +    return dovi;
> +}
> diff --git a/libavutil/dovi_meta.h b/libavutil/dovi_meta.h
> index 299911d434..20efd41676 100644
> --- a/libavutil/dovi_meta.h
> +++ b/libavutil/dovi_meta.h
> @@ -29,6 +29,7 @@
>  
>  #include <stdint.h>
>  #include <stddef.h>
> +#include "rational.h"
>  
>  /*
>   * DOVI configuration
> @@ -67,4 +68,135 @@ typedef struct AVDOVIDecoderConfigurationRecord {
>   */
>  AVDOVIDecoderConfigurationRecord *av_dovi_alloc(size_t *size);
>  
> +/**
> + * Dolby Vision RPU data header.
> + */
> +typedef struct AVDOVIRpuDataHeader {
> +    uint8_t rpu_type;
> +    uint16_t rpu_format;
> +    uint8_t vdr_rpu_profile;
> +    uint8_t vdr_rpu_level;
> +    int chroma_resampling_explicit_filter_flag;

Why are flags ints, while other things can be uint8_t?

> +
> +/**
> + * Combined struct representing a combination of header, mapping and color
> + * metadata, for attaching to frames as side data.
> + *
> + * @note The struct must be allocated with av_dovi_metadata_alloc() and
> + *       its size is not a part of the public ABI.

Maybe mention that the individual structs (header, mapping, color)
CANNOT be extended without a major bump.

> + */
> +
> +typedef struct AVDOVIMetadata {
> +    AVDOVIRpuDataHeader header;
> +    AVDOVIDataMapping mapping;
> +    AVDOVIColorMetadata color;
> +} AVDOVIMetadata;
> +
> +/**
> + * Allocate an AVDOVIMetadata structure and initialize its
> + * fields to default values.

+ * @param size If this parameter is non-NULL, the size in bytes of the
                allocated struct will be written here on success

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 4/6] lavc: Implement Dolby Vision RPU parsing

[Anton Khirnov]

Quoting Niklas Haas (2021-12-12 12:55:44)
> +void ff_dovi_ctx_unref(DOVIContext *s)
> +{
> +    for (int i = 0; i < DOVI_MAX_DM_ID; i++)

nit: FF_ARRAY_ELEMS(s->vdr_ref) is considered better

> +        av_buffer_unref(&s->vdr_ref[i]);
> +
> +    /* Preserve the user-provided fields explicitly, reset everything else */
> +    *s = (DOVIContext) {
> +        .avctx = s->avctx,
> +        .config = s->config,
> +    };
> +}
> +
> +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
> +{
> +    int ret;
> +    s->avctx = s0->avctx;
> +    s->config = s0->config;
> +    s->mapping = s0->mapping;
> +    s->color = s0->color;
> +    for (int i = 0; i < DOVI_MAX_DM_ID; i++) {
> +        if ((ret = av_buffer_replace(&s->vdr_ref[i], s0->vdr_ref[i])) < 0)
> +            goto fail;
> +    }
> +
> +    return 0;
> +
> +fail:
> +    /* clean up any potentially invalid pointers */

Would not calling ff_dovi_ctx_unref() be safer here?

> +    s->mapping = NULL;
> +    s->color = NULL;
> +    return ret;
> +}
> [...]
> diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h
> new file mode 100644
> index 0000000000..8cabff2718
> --- /dev/null
> +++ b/libavcodec/dovi_rpu.h
> @@ -0,0 +1,71 @@
> +/*
> + * Dolby Vision RPU decoder
> + *
> + * Copyright (C) 2021 Jan Ekström
> + * Copyright (C) 2021 Niklas Haas
> + *
> + * 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
> + */
> +
> +#ifndef AVCODEC_DOVI_RPU_H
> +#define AVCODEC_DOVI_RPU_H
> +
> +#include "libavutil/dovi_meta.h"
> +#include "avcodec.h"
> +
> +#define DOVI_MAX_DM_ID 15
> +typedef struct DOVIContext {
> +    AVCodecContext *avctx;

This is only used for logging (as it should be), so you can make this
void *logctx and drop the avcodec.h #include.

-- 
Anton Khirnov
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 4/6] lavc: Implement Dolby Vision RPU parsing

[Niklas Haas]

On Mon, 20 Dec 2021 11:13:15 +0100 Anton Khirnov <anton@khirnov.net> wrote:
> nit: FF_ARRAY_ELEMS(s->vdr_ref) is considered better

Done.

> Would not calling ff_dovi_ctx_unref() be safer here?

Good point, changed.

> This is only used for logging (as it should be), so you can make this
> void *logctx and drop the avcodec.h #include.

So, the reason I made this an avctx is because I'm anticipating the need
to check `avctx->err_recognition` if the CRC32 check ever gets
implemented. In theory we could also do some profile-based verification
in AV_EF_BITSTREAM / AV_EF_EXPLODE, to check if the coded RPU values
actually match the legal combinations restricted by the profile
specification.

Of course, given that this is just an internal API, I'm also fine making
it a logctx now and upgrading it to an avctx when the need arises.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v5 4/6] lavc: Implement Dolby Vision RPU parsing

[Anton Khirnov]

Quoting Niklas Haas (2021-12-20 15:47:45)
> So, the reason I made this an avctx is because I'm anticipating the need
> to check `avctx->err_recognition` if the CRC32 check ever gets
> implemented. In theory we could also do some profile-based verification
> in AV_EF_BITSTREAM / AV_EF_EXPLODE, to check if the coded RPU values
> actually match the legal combinations restricted by the profile
> specification.

I would much prefer this single flag to be passed in as a parameter,
given how much of a god object AVCodecContext already is.

-- 
Anton Khirnov
_______________________________________________
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".