[FFmpeg-devel,v9,4/6] lavc: Implement Dolby Vision RPU parsing

From: Niklas Haas <git@haasn.dev>

Based on a mixture of guesswork, partial documentation in patents, and
reverse engineering of real-world samples. Confirmed working for all the
samples I've thrown at it.

Contains some annoying machinery to persist these values in between
frames, which is needed in theory even though I've never actually seen a
sample that relies on it in practice. May or may not work.

Since the distinction matters greatly for parsing the color matrix
values, this includes a small helper function to guess the right profile
from the RPU itself in case the user has forgotten to forward the dovi
configuration record to the decoder. (Which in practice, only ffmpeg.c
and ffplay do..)

Notable omissions / deviations:
- CRC32 verification. This is based on the MPEG2 CRC32 type, which does
  not seem to be implemented in lavu. (And I don't care enough to do so)
- Linear interpolation support. Nothing documents this (beyond its
  existence) and no samples use it, so impossible to implement.
- All of the extension metadata blocks, but these contain values that
  seem largely congruent with ST2094, HDR10, or other existing forms of
  side data, so I will defer parsing/attaching them to a future commit.
- The patent describes a mechanism for predicting coefficients from
  previous RPUs, but the bit for the flag whether to use the
  prediction deltas or signal entirely new coefficients does not seem to
  be present in actual RPUs, so we ignore this subsystem entirely.
- In the patent's spec, the NLQ subsystem also loops over
  num_nlq_pivots, but even in the patent the number is hard-coded to one
  iteration rather than signalled. So we only store one set of coefs.

Heavily influenced by https://github.com/quietvoid/dovi_tool
Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001

Signed-off-by: Niklas Haas <git@haasn.dev>
---
 configure             |   2 +
 libavcodec/Makefile   |   1 +
 libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
 libavcodec/dovi_rpu.h |  71 +++++++
 4 files changed, 504 insertions(+)
 create mode 100644 libavcodec/dovi_rpu.c
 create mode 100644 libavcodec/dovi_rpu.h

Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> Based on a mixture of guesswork, partial documentation in patents, and
> reverse engineering of real-world samples. Confirmed working for all the
> samples I've thrown at it.
> 
> Contains some annoying machinery to persist these values in between
> frames, which is needed in theory even though I've never actually seen a
> sample that relies on it in practice. May or may not work.
> 
> Since the distinction matters greatly for parsing the color matrix
> values, this includes a small helper function to guess the right profile
> from the RPU itself in case the user has forgotten to forward the dovi
> configuration record to the decoder. (Which in practice, only ffmpeg.c
> and ffplay do..)
> 
> Notable omissions / deviations:
> - CRC32 verification. This is based on the MPEG2 CRC32 type, which does
>   not seem to be implemented in lavu. (And I don't care enough to do so)
> - Linear interpolation support. Nothing documents this (beyond its
>   existence) and no samples use it, so impossible to implement.
> - All of the extension metadata blocks, but these contain values that
>   seem largely congruent with ST2094, HDR10, or other existing forms of
>   side data, so I will defer parsing/attaching them to a future commit.
> - The patent describes a mechanism for predicting coefficients from
>   previous RPUs, but the bit for the flag whether to use the
>   prediction deltas or signal entirely new coefficients does not seem to
>   be present in actual RPUs, so we ignore this subsystem entirely.
> - In the patent's spec, the NLQ subsystem also loops over
>   num_nlq_pivots, but even in the patent the number is hard-coded to one
>   iteration rather than signalled. So we only store one set of coefs.
> 
> Heavily influenced by https://github.com/quietvoid/dovi_tool
> Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001
> 
> Signed-off-by: Niklas Haas <git@haasn.dev>
> ---
>  configure             |   2 +
>  libavcodec/Makefile   |   1 +
>  libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/dovi_rpu.h |  71 +++++++
>  4 files changed, 504 insertions(+)
>  create mode 100644 libavcodec/dovi_rpu.c
>  create mode 100644 libavcodec/dovi_rpu.h
> 
> diff --git a/configure b/configure
> index 0ccd3bda11..68658a847f 100755
> --- a/configure
> +++ b/configure
> @@ -2434,6 +2434,7 @@ CONFIG_EXTRA="
>      cbs_vp9
>      dirac_parse
>      dnn
> +    dovi_rpu
>      dvprofile
>      exif
>      faandct
> @@ -2706,6 +2707,7 @@ cbs_mpeg2_select="cbs"
>  cbs_vp9_select="cbs"
>  dct_select="rdft"
>  dirac_parse_select="golomb"
> +dovi_rpu_select="golomb"
>  dnn_suggest="libtensorflow libopenvino"
>  dnn_deps="avformat swscale"
>  error_resilience_select="me_cmp"
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index fb90ecea84..7364c7a91f 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -77,6 +77,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
> +OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
>  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
>  OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
>  OBJS-$(CONFIG_FAANDCT)                 += faandct.o
> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
> new file mode 100644
> index 0000000000..fc2e1fb2a1
> --- /dev/null
> +++ b/libavcodec/dovi_rpu.c
> @@ -0,0 +1,430 @@
> +/*
> + * 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
> + */
> +
> +#include "libavutil/buffer.h"
> +
> +#include "dovi_rpu.h"
> +#include "golomb.h"
> +#include "get_bits.h"
> +
> +enum {
> +    RPU_COEFF_FIXED = 0,
> +    RPU_COEFF_FLOAT = 1,
> +};
> +
> +/**
> + * Private contents of vdr_ref.
> + */
> +typedef struct DOVIVdrRef {
> +    AVDOVIDataMapping mapping;
> +    AVDOVIColorMetadata color;
> +} DOVIVdrRef;
> +
> +void ff_dovi_ctx_unref(DOVIContext *s)
> +{
> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr_ref); i++)
> +        av_buffer_unref(&s->vdr_ref[i]);
> +
> +    /* Preserve the user-provided fields explicitly, reset everything else */
> +    *s = (DOVIContext) {
> +        .logctx = s->logctx,
> +        .config = s->config,
> +    };
> +}
> +
> +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
> +{
> +    int ret;
> +    s->logctx = s0->logctx;
> +    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:
> +    ff_dovi_ctx_unref(s);
> +    return ret;
> +}
> +
> +int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame)
> +{
> +    AVFrameSideData *sd;
> +    AVBufferRef *buf;
> +    AVDOVIMetadata *dovi;
> +    size_t dovi_size;
> +
> +    if (!s->mapping || !s->color)
> +        return 0; /* incomplete dovi metadata */
> +
> +    dovi = av_dovi_metadata_alloc(&dovi_size);
> +    if (!dovi)
> +        return AVERROR(ENOMEM);
> +
> +    buf = av_buffer_create((uint8_t *) dovi, dovi_size, NULL, NULL, 0);
> +    if (!buf) {
> +        av_free(dovi);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    sd = av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_DOVI_METADATA, buf);
> +    if (!sd) {
> +        av_buffer_unref(&buf);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    memcpy(av_dovi_get_header(dovi), &s->header, sizeof(s->header));
> +    memcpy(av_dovi_get_mapping(dovi), s->mapping, sizeof(*s->mapping));
> +    memcpy(av_dovi_get_color(dovi), s->color, sizeof(*s->color));

These are potentially problematic due to trailing padding in the
structures. This trailing padding might become valid fields in newer
versions of this structure. E.g. av_dovi_metadata_alloc() might
intentionally set the default values of these new fields to invalid
values. Yet lavc (with the old structure) sees them as padding, allowing
the compiler to trash these values on every write to these structures.
More likely though is that they will never be touched at all and
therefore retain their default (zero) initialization. Which might
conflict with the default value of these fields.
This can be fixed by only copying everything up to and including the
last field that we know of for every of these structures. This will
unfortunately increase the maintainence burden a bit.

> +    return 0;
> +}
> +
> +static int guess_profile(const AVDOVIRpuDataHeader *hdr)
> +{
> +    switch (hdr->vdr_rpu_profile) {
> +    case 0:
> +        if (hdr->bl_video_full_range_flag)
> +            return 5;
> +        break;
> +    case 1:
> +        if (hdr->el_spatial_resampling_filter_flag && !hdr->disable_residual_flag) {
> +            if (hdr->vdr_bit_depth == 12) {
> +                return 7;
> +            } else {
> +                return 4;
> +            }
> +        } else {
> +            return 8;
> +        }
> +    }
> +
> +    return 0; /* unknown */
> +}
> +
> +static inline uint64_t get_ue_coef(GetBitContext *gb, const AVDOVIRpuDataHeader *hdr)
> +{
> +    uint64_t ipart;
> +    union { uint32_t u32; float f32; } fpart;
> +
> +    switch (hdr->coef_data_type) {
> +    case RPU_COEFF_FIXED:
> +        ipart = get_ue_golomb_long(gb);
> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;

Here and in get_se_coef below the f32 member of the union is not used
with RPU_COEFF_FIXED, so using the union seems unnecessary. It is also
slightly confusing, because upon seeing this and seeing that the union
is used with RPU_COEFF_FIXED, I expect it to be used for type-punning.

> +
> +    case RPU_COEFF_FLOAT:
> +        fpart.u32 = get_bits_long(gb, 32);
> +        return fpart.f32 * (1 << hdr->coef_log2_denom);

You could just use av_int2float() here instead of adding the union yourself.

> +    }
> +
> +    return 0; /* unreachable */
> +}
> +
> +static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader *hdr)
> +{
> +    int64_t ipart;
> +    union { uint32_t u32; float f32; } fpart;
> +
> +    switch (hdr->coef_data_type) {
> +    case RPU_COEFF_FIXED:
> +        ipart = get_se_golomb_long(gb);
> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
> +
> +    case RPU_COEFF_FLOAT:
> +        fpart.u32 = get_bits_long(gb, 32);
> +        return fpart.f32 * (1 << hdr->coef_log2_denom);
> +    }
> +
> +    return 0; /* unreachable */
> +}
> +
> +#define VALIDATE(VAR, MIN, MAX)                                                 \
> +    do {                                                                        \
> +        if (VAR < MIN || VAR > MAX) {                                           \
> +            av_log(s->logctx, AV_LOG_ERROR, "RPU validation failed: "           \
> +                   #MIN" <= "#VAR" = %d <= "#MAX"\n", (int) VAR);               \
> +            goto fail;                                                          \
> +        }                                                                       \
> +    } while (0)
> +
> +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size)
> +{
> +    AVDOVIRpuDataHeader *hdr = &s->header;
> +    GetBitContext *gb = &(GetBitContext){0};
> +    DOVIVdrRef *vdr;
> +    int ret;
> +
> +    uint8_t nal_prefix;
> +    uint8_t rpu_type;
> +    uint8_t vdr_seq_info_present;
> +    uint8_t vdr_dm_metadata_present;
> +    uint8_t use_prev_vdr_rpu;
> +    uint8_t use_nlq;
> +    uint8_t profile;
> +    if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0)

Notice that rpu_size is converted to int before init_get_bits8() is
called. (It is not problematic with the use in 6/6 because the size has
already been checked in this case.)

> +        return ret;
> +

On 1/2/2022 1:18 AM, Andreas Rheinhardt wrote:
> Niklas Haas:
>> From: Niklas Haas <git@haasn.dev>
>>
>> Based on a mixture of guesswork, partial documentation in patents, and
>> reverse engineering of real-world samples. Confirmed working for all the
>> samples I've thrown at it.
>>
>> Contains some annoying machinery to persist these values in between
>> frames, which is needed in theory even though I've never actually seen a
>> sample that relies on it in practice. May or may not work.
>>
>> Since the distinction matters greatly for parsing the color matrix
>> values, this includes a small helper function to guess the right profile
>> from the RPU itself in case the user has forgotten to forward the dovi
>> configuration record to the decoder. (Which in practice, only ffmpeg.c
>> and ffplay do..)
>>
>> Notable omissions / deviations:
>> - CRC32 verification. This is based on the MPEG2 CRC32 type, which does
>>    not seem to be implemented in lavu. (And I don't care enough to do so)
>> - Linear interpolation support. Nothing documents this (beyond its
>>    existence) and no samples use it, so impossible to implement.
>> - All of the extension metadata blocks, but these contain values that
>>    seem largely congruent with ST2094, HDR10, or other existing forms of
>>    side data, so I will defer parsing/attaching them to a future commit.
>> - The patent describes a mechanism for predicting coefficients from
>>    previous RPUs, but the bit for the flag whether to use the
>>    prediction deltas or signal entirely new coefficients does not seem to
>>    be present in actual RPUs, so we ignore this subsystem entirely.
>> - In the patent's spec, the NLQ subsystem also loops over
>>    num_nlq_pivots, but even in the patent the number is hard-coded to one
>>    iteration rather than signalled. So we only store one set of coefs.
>>
>> Heavily influenced by https://github.com/quietvoid/dovi_tool
>> Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001
>>
>> Signed-off-by: Niklas Haas <git@haasn.dev>
>> ---
>>   configure             |   2 +
>>   libavcodec/Makefile   |   1 +
>>   libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
>>   libavcodec/dovi_rpu.h |  71 +++++++
>>   4 files changed, 504 insertions(+)
>>   create mode 100644 libavcodec/dovi_rpu.c
>>   create mode 100644 libavcodec/dovi_rpu.h
>>
>> diff --git a/configure b/configure
>> index 0ccd3bda11..68658a847f 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2434,6 +2434,7 @@ CONFIG_EXTRA="
>>       cbs_vp9
>>       dirac_parse
>>       dnn
>> +    dovi_rpu
>>       dvprofile
>>       exif
>>       faandct
>> @@ -2706,6 +2707,7 @@ cbs_mpeg2_select="cbs"
>>   cbs_vp9_select="cbs"
>>   dct_select="rdft"
>>   dirac_parse_select="golomb"
>> +dovi_rpu_select="golomb"
>>   dnn_suggest="libtensorflow libopenvino"
>>   dnn_deps="avformat swscale"
>>   error_resilience_select="me_cmp"
>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>> index fb90ecea84..7364c7a91f 100644
>> --- a/libavcodec/Makefile
>> +++ b/libavcodec/Makefile
>> @@ -77,6 +77,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>>   OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
>>   OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>>   OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o dct32_float.o
>> +OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
>>   OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
>>   OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
>>   OBJS-$(CONFIG_FAANDCT)                 += faandct.o
>> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
>> new file mode 100644
>> index 0000000000..fc2e1fb2a1
>> --- /dev/null
>> +++ b/libavcodec/dovi_rpu.c
>> @@ -0,0 +1,430 @@
>> +/*
>> + * 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
>> + */
>> +
>> +#include "libavutil/buffer.h"
>> +
>> +#include "dovi_rpu.h"
>> +#include "golomb.h"
>> +#include "get_bits.h"
>> +
>> +enum {
>> +    RPU_COEFF_FIXED = 0,
>> +    RPU_COEFF_FLOAT = 1,
>> +};
>> +
>> +/**
>> + * Private contents of vdr_ref.
>> + */
>> +typedef struct DOVIVdrRef {
>> +    AVDOVIDataMapping mapping;
>> +    AVDOVIColorMetadata color;
>> +} DOVIVdrRef;
>> +
>> +void ff_dovi_ctx_unref(DOVIContext *s)
>> +{
>> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr_ref); i++)
>> +        av_buffer_unref(&s->vdr_ref[i]);
>> +
>> +    /* Preserve the user-provided fields explicitly, reset everything else */
>> +    *s = (DOVIContext) {
>> +        .logctx = s->logctx,
>> +        .config = s->config,
>> +    };
>> +}
>> +
>> +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
>> +{
>> +    int ret;
>> +    s->logctx = s0->logctx;
>> +    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:
>> +    ff_dovi_ctx_unref(s);
>> +    return ret;
>> +}
>> +
>> +int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame)
>> +{
>> +    AVFrameSideData *sd;
>> +    AVBufferRef *buf;
>> +    AVDOVIMetadata *dovi;
>> +    size_t dovi_size;
>> +
>> +    if (!s->mapping || !s->color)
>> +        return 0; /* incomplete dovi metadata */
>> +
>> +    dovi = av_dovi_metadata_alloc(&dovi_size);
>> +    if (!dovi)
>> +        return AVERROR(ENOMEM);
>> +
>> +    buf = av_buffer_create((uint8_t *) dovi, dovi_size, NULL, NULL, 0);
>> +    if (!buf) {
>> +        av_free(dovi);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    sd = av_frame_new_side_data_from_buf(frame, AV_FRAME_DATA_DOVI_METADATA, buf);
>> +    if (!sd) {
>> +        av_buffer_unref(&buf);
>> +        return AVERROR(ENOMEM);
>> +    }
>> +
>> +    memcpy(av_dovi_get_header(dovi), &s->header, sizeof(s->header));
>> +    memcpy(av_dovi_get_mapping(dovi), s->mapping, sizeof(*s->mapping));
>> +    memcpy(av_dovi_get_color(dovi), s->color, sizeof(*s->color));
> 
> These are potentially problematic due to trailing padding in the
> structures. This trailing padding might become valid fields in newer
> versions of this structure. E.g. av_dovi_metadata_alloc() might
> intentionally set the default values of these new fields to invalid
> values. Yet lavc (with the old structure) sees them as padding, allowing
> the compiler to trash these values on every write to these structures.
> More likely though is that they will never be touched at all and
> therefore retain their default (zero) initialization. Which might
> conflict with the default value of these fields.
> This can be fixed by only copying everything up to and including the
> last field that we know of for every of these structures. This will
> unfortunately increase the maintainence burden a bit.

I mean, both sizeof(AVDOVIRpuDataHeader) and sizeof(AVDOVIDataMapping) 
are defined as not being part of the ABI (but 
sizeof(AVDOVIColorMetadata) is? Is that intended?), so their usage in 
this patch is wrong to begin with.

> 
>> +    return 0;
>> +}
>> +
>> +static int guess_profile(const AVDOVIRpuDataHeader *hdr)
>> +{
>> +    switch (hdr->vdr_rpu_profile) {
>> +    case 0:
>> +        if (hdr->bl_video_full_range_flag)
>> +            return 5;
>> +        break;
>> +    case 1:
>> +        if (hdr->el_spatial_resampling_filter_flag && !hdr->disable_residual_flag) {
>> +            if (hdr->vdr_bit_depth == 12) {
>> +                return 7;
>> +            } else {
>> +                return 4;
>> +            }
>> +        } else {
>> +            return 8;
>> +        }
>> +    }
>> +
>> +    return 0; /* unknown */
>> +}
>> +
>> +static inline uint64_t get_ue_coef(GetBitContext *gb, const AVDOVIRpuDataHeader *hdr)
>> +{
>> +    uint64_t ipart;
>> +    union { uint32_t u32; float f32; } fpart;
>> +
>> +    switch (hdr->coef_data_type) {
>> +    case RPU_COEFF_FIXED:
>> +        ipart = get_ue_golomb_long(gb);
>> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
>> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
> 
> Here and in get_se_coef below the f32 member of the union is not used
> with RPU_COEFF_FIXED, so using the union seems unnecessary. It is also
> slightly confusing, because upon seeing this and seeing that the union
> is used with RPU_COEFF_FIXED, I expect it to be used for type-punning.
> 
>> +
>> +    case RPU_COEFF_FLOAT:
>> +        fpart.u32 = get_bits_long(gb, 32);
>> +        return fpart.f32 * (1 << hdr->coef_log2_denom);
> 
> You could just use av_int2float() here instead of adding the union yourself.
> 
>> +    }
>> +
>> +    return 0; /* unreachable */
>> +}
>> +
>> +static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader *hdr)
>> +{
>> +    int64_t ipart;
>> +    union { uint32_t u32; float f32; } fpart;
>> +
>> +    switch (hdr->coef_data_type) {
>> +    case RPU_COEFF_FIXED:
>> +        ipart = get_se_golomb_long(gb);
>> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
>> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
>> +
>> +    case RPU_COEFF_FLOAT:
>> +        fpart.u32 = get_bits_long(gb, 32);
>> +        return fpart.f32 * (1 << hdr->coef_log2_denom);
>> +    }
>> +
>> +    return 0; /* unreachable */
>> +}
>> +
>> +#define VALIDATE(VAR, MIN, MAX)                                                 \
>> +    do {                                                                        \
>> +        if (VAR < MIN || VAR > MAX) {                                           \
>> +            av_log(s->logctx, AV_LOG_ERROR, "RPU validation failed: "           \
>> +                   #MIN" <= "#VAR" = %d <= "#MAX"\n", (int) VAR);               \
>> +            goto fail;                                                          \
>> +        }                                                                       \
>> +    } while (0)
>> +
>> +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size)
>> +{
>> +    AVDOVIRpuDataHeader *hdr = &s->header;
>> +    GetBitContext *gb = &(GetBitContext){0};
>> +    DOVIVdrRef *vdr;
>> +    int ret;
>> +
>> +    uint8_t nal_prefix;
>> +    uint8_t rpu_type;
>> +    uint8_t vdr_seq_info_present;
>> +    uint8_t vdr_dm_metadata_present;
>> +    uint8_t use_prev_vdr_rpu;
>> +    uint8_t use_nlq;
>> +    uint8_t profile;
>> +    if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0)
> 
> Notice that rpu_size is converted to int before init_get_bits8() is
> called. (It is not problematic with the use in 6/6 because the size has
> already been checked in this case.)
> 
>> +        return ret;
>> +
> _______________________________________________
> 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".

James Almer <jamrial@gmail.com> schrieb am So., 2. Jan. 2022, 13:46:

> On 1/2/2022 1:18 AM, Andreas Rheinhardt wrote:
> > Niklas Haas:
> >> From: Niklas Haas <git@haasn.dev>
> >>
> >> Based on a mixture of guesswork, partial documentation in patents, and
> >> reverse engineering of real-world samples. Confirmed working for all the
> >> samples I've thrown at it.
> >>
> >> Contains some annoying machinery to persist these values in between
> >> frames, which is needed in theory even though I've never actually seen a
> >> sample that relies on it in practice. May or may not work.
> >>
> >> Since the distinction matters greatly for parsing the color matrix
> >> values, this includes a small helper function to guess the right profile
> >> from the RPU itself in case the user has forgotten to forward the dovi
> >> configuration record to the decoder. (Which in practice, only ffmpeg.c
> >> and ffplay do..)
> >>
> >> Notable omissions / deviations:
> >> - CRC32 verification. This is based on the MPEG2 CRC32 type, which does
> >>    not seem to be implemented in lavu. (And I don't care enough to do
> so)
> >> - Linear interpolation support. Nothing documents this (beyond its
> >>    existence) and no samples use it, so impossible to implement.
> >> - All of the extension metadata blocks, but these contain values that
> >>    seem largely congruent with ST2094, HDR10, or other existing forms of
> >>    side data, so I will defer parsing/attaching them to a future commit.
> >> - The patent describes a mechanism for predicting coefficients from
> >>    previous RPUs, but the bit for the flag whether to use the
> >>    prediction deltas or signal entirely new coefficients does not seem
> to
> >>    be present in actual RPUs, so we ignore this subsystem entirely.
> >> - In the patent's spec, the NLQ subsystem also loops over
> >>    num_nlq_pivots, but even in the patent the number is hard-coded to
> one
> >>    iteration rather than signalled. So we only store one set of coefs.
> >>
> >> Heavily influenced by https://github.com/quietvoid/dovi_tool
> >> Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001
> >>
> >> Signed-off-by: Niklas Haas <git@haasn.dev>
> >> ---
> >>   configure             |   2 +
> >>   libavcodec/Makefile   |   1 +
> >>   libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
> >>   libavcodec/dovi_rpu.h |  71 +++++++
> >>   4 files changed, 504 insertions(+)
> >>   create mode 100644 libavcodec/dovi_rpu.c
> >>   create mode 100644 libavcodec/dovi_rpu.h
> >>
> >> diff --git a/configure b/configure
> >> index 0ccd3bda11..68658a847f 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -2434,6 +2434,7 @@ CONFIG_EXTRA="
> >>       cbs_vp9
> >>       dirac_parse
> >>       dnn
> >> +    dovi_rpu
> >>       dvprofile
> >>       exif
> >>       faandct
> >> @@ -2706,6 +2707,7 @@ cbs_mpeg2_select="cbs"
> >>   cbs_vp9_select="cbs"
> >>   dct_select="rdft"
> >>   dirac_parse_select="golomb"
> >> +dovi_rpu_select="golomb"
> >>   dnn_suggest="libtensorflow libopenvino"
> >>   dnn_deps="avformat swscale"
> >>   error_resilience_select="me_cmp"
> >> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> >> index fb90ecea84..7364c7a91f 100644
> >> --- a/libavcodec/Makefile
> >> +++ b/libavcodec/Makefile
> >> @@ -77,6 +77,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
> >>   OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> >>   OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
> >>   OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o
> dct32_float.o
> >> +OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
> >>   OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
> >>   OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
> >>   OBJS-$(CONFIG_FAANDCT)                 += faandct.o
> >> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
> >> new file mode 100644
> >> index 0000000000..fc2e1fb2a1
> >> --- /dev/null
> >> +++ b/libavcodec/dovi_rpu.c
> >> @@ -0,0 +1,430 @@
> >> +/*
> >> + * 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
> >> + */
> >> +
> >> +#include "libavutil/buffer.h"
> >> +
> >> +#include "dovi_rpu.h"
> >> +#include "golomb.h"
> >> +#include "get_bits.h"
> >> +
> >> +enum {
> >> +    RPU_COEFF_FIXED = 0,
> >> +    RPU_COEFF_FLOAT = 1,
> >> +};
> >> +
> >> +/**
> >> + * Private contents of vdr_ref.
> >> + */
> >> +typedef struct DOVIVdrRef {
> >> +    AVDOVIDataMapping mapping;
> >> +    AVDOVIColorMetadata color;
> >> +} DOVIVdrRef;
> >> +
> >> +void ff_dovi_ctx_unref(DOVIContext *s)
> >> +{
> >> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr_ref); i++)
> >> +        av_buffer_unref(&s->vdr_ref[i]);
> >> +
> >> +    /* Preserve the user-provided fields explicitly, reset everything
> else */
> >> +    *s = (DOVIContext) {
> >> +        .logctx = s->logctx,
> >> +        .config = s->config,
> >> +    };
> >> +}
> >> +
> >> +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
> >> +{
> >> +    int ret;
> >> +    s->logctx = s0->logctx;
> >> +    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:
> >> +    ff_dovi_ctx_unref(s);
> >> +    return ret;
> >> +}
> >> +
> >> +int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame)
> >> +{
> >> +    AVFrameSideData *sd;
> >> +    AVBufferRef *buf;
> >> +    AVDOVIMetadata *dovi;
> >> +    size_t dovi_size;
> >> +
> >> +    if (!s->mapping || !s->color)
> >> +        return 0; /* incomplete dovi metadata */
> >> +
> >> +    dovi = av_dovi_metadata_alloc(&dovi_size);
> >> +    if (!dovi)
> >> +        return AVERROR(ENOMEM);
> >> +
> >> +    buf = av_buffer_create((uint8_t *) dovi, dovi_size, NULL, NULL, 0);
> >> +    if (!buf) {
> >> +        av_free(dovi);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    sd = av_frame_new_side_data_from_buf(frame,
> AV_FRAME_DATA_DOVI_METADATA, buf);
> >> +    if (!sd) {
> >> +        av_buffer_unref(&buf);
> >> +        return AVERROR(ENOMEM);
> >> +    }
> >> +
> >> +    memcpy(av_dovi_get_header(dovi), &s->header, sizeof(s->header));
> >> +    memcpy(av_dovi_get_mapping(dovi), s->mapping, sizeof(*s->mapping));
> >> +    memcpy(av_dovi_get_color(dovi), s->color, sizeof(*s->color));
> >
> > These are potentially problematic due to trailing padding in the
> > structures. This trailing padding might become valid fields in newer
> > versions of this structure. E.g. av_dovi_metadata_alloc() might
> > intentionally set the default values of these new fields to invalid
> > values. Yet lavc (with the old structure) sees them as padding, allowing
> > the compiler to trash these values on every write to these structures.
> > More likely though is that they will never be touched at all and
> > therefore retain their default (zero) initialization. Which might
> > conflict with the default value of these fields.
> > This can be fixed by only copying everything up to and including the
> > last field that we know of for every of these structures. This will
> > unfortunately increase the maintainence burden a bit.


> I mean, both sizeof(AVDOVIRpuDataHeader) and sizeof(AVDOVIDataMapping)
> are defined as not being part of the ABI (but
> sizeof(AVDOVIColorMetadata) is? Is that intended?), so their usage in
> this patch is wrong to begin with.
>

This is not actually a problem since it doesn't rely on a stable ABI for
the size, but only knowing how big the struct was it was built with, and
filled itself. If the struct grows in size in lavu then the remainder will
just be left alone, since every struct is a distinct pointer into the
overall buffer.


> >
> >> +    return 0;
> >> +}
> >> +
> >> +static int guess_profile(const AVDOVIRpuDataHeader *hdr)
> >> +{
> >> +    switch (hdr->vdr_rpu_profile) {
> >> +    case 0:
> >> +        if (hdr->bl_video_full_range_flag)
> >> +            return 5;
> >> +        break;
> >> +    case 1:
> >> +        if (hdr->el_spatial_resampling_filter_flag &&
> !hdr->disable_residual_flag) {
> >> +            if (hdr->vdr_bit_depth == 12) {
> >> +                return 7;
> >> +            } else {
> >> +                return 4;
> >> +            }
> >> +        } else {
> >> +            return 8;
> >> +        }
> >> +    }
> >> +
> >> +    return 0; /* unknown */
> >> +}
> >> +
> >> +static inline uint64_t get_ue_coef(GetBitContext *gb, const
> AVDOVIRpuDataHeader *hdr)
> >> +{
> >> +    uint64_t ipart;
> >> +    union { uint32_t u32; float f32; } fpart;
> >> +
> >> +    switch (hdr->coef_data_type) {
> >> +    case RPU_COEFF_FIXED:
> >> +        ipart = get_ue_golomb_long(gb);
> >> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> >> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
> >
> > Here and in get_se_coef below the f32 member of the union is not used
> > with RPU_COEFF_FIXED, so using the union seems unnecessary. It is also
> > slightly confusing, because upon seeing this and seeing that the union
> > is used with RPU_COEFF_FIXED, I expect it to be used for type-punning.
> >
> >> +
> >> +    case RPU_COEFF_FLOAT:
> >> +        fpart.u32 = get_bits_long(gb, 32);
> >> +        return fpart.f32 * (1 << hdr->coef_log2_denom);
> >
> > You could just use av_int2float() here instead of adding the union
> yourself.
> >
> >> +    }
> >> +
> >> +    return 0; /* unreachable */
> >> +}
> >> +
> >> +static inline int64_t get_se_coef(GetBitContext *gb, const
> AVDOVIRpuDataHeader *hdr)
> >> +{
> >> +    int64_t ipart;
> >> +    union { uint32_t u32; float f32; } fpart;
> >> +
> >> +    switch (hdr->coef_data_type) {
> >> +    case RPU_COEFF_FIXED:
> >> +        ipart = get_se_golomb_long(gb);
> >> +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> >> +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
> >> +
> >> +    case RPU_COEFF_FLOAT:
> >> +        fpart.u32 = get_bits_long(gb, 32);
> >> +        return fpart.f32 * (1 << hdr->coef_log2_denom);
> >> +    }
> >> +
> >> +    return 0; /* unreachable */
> >> +}
> >> +
> >> +#define VALIDATE(VAR, MIN, MAX)
>          \
> >> +    do {
>           \
> >> +        if (VAR < MIN || VAR > MAX) {
>          \
> >> +            av_log(s->logctx, AV_LOG_ERROR, "RPU validation failed: "
>          \
> >> +                   #MIN" <= "#VAR" = %d <= "#MAX"\n", (int) VAR);
>          \
> >> +            goto fail;
>           \
> >> +        }
>          \
> >> +    } while (0)
> >> +
> >> +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t
> rpu_size)
> >> +{
> >> +    AVDOVIRpuDataHeader *hdr = &s->header;
> >> +    GetBitContext *gb = &(GetBitContext){0};
> >> +    DOVIVdrRef *vdr;
> >> +    int ret;
> >> +
> >> +    uint8_t nal_prefix;
> >> +    uint8_t rpu_type;
> >> +    uint8_t vdr_seq_info_present;
> >> +    uint8_t vdr_dm_metadata_present;
> >> +    uint8_t use_prev_vdr_rpu;
> >> +    uint8_t use_nlq;
> >> +    uint8_t profile;
> >> +    if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0)
> >
> > Notice that rpu_size is converted to int before init_get_bits8() is
> > called. (It is not problematic with the use in 6/6 because the size has
> > already been checked in this case.)
> >
> >> +        return ret;
> >> +
> > _______________________________________________
> > 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".
>
> _______________________________________________
> 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".
>

Andreas Rheinhardt <andreas.rheinhardt@outlook.com> schrieb am So., 2. Jan.
2022, 05:18:

> Niklas Haas:
> > From: Niklas Haas <git@haasn.dev>
> >
> > Based on a mixture of guesswork, partial documentation in patents, and
> > reverse engineering of real-world samples. Confirmed working for all the
> > samples I've thrown at it.
> >
> > Contains some annoying machinery to persist these values in between
> > frames, which is needed in theory even though I've never actually seen a
> > sample that relies on it in practice. May or may not work.
> >
> > Since the distinction matters greatly for parsing the color matrix
> > values, this includes a small helper function to guess the right profile
> > from the RPU itself in case the user has forgotten to forward the dovi
> > configuration record to the decoder. (Which in practice, only ffmpeg.c
> > and ffplay do..)
> >
> > Notable omissions / deviations:
> > - CRC32 verification. This is based on the MPEG2 CRC32 type, which does
> >   not seem to be implemented in lavu. (And I don't care enough to do so)
> > - Linear interpolation support. Nothing documents this (beyond its
> >   existence) and no samples use it, so impossible to implement.
> > - All of the extension metadata blocks, but these contain values that
> >   seem largely congruent with ST2094, HDR10, or other existing forms of
> >   side data, so I will defer parsing/attaching them to a future commit.
> > - The patent describes a mechanism for predicting coefficients from
> >   previous RPUs, but the bit for the flag whether to use the
> >   prediction deltas or signal entirely new coefficients does not seem to
> >   be present in actual RPUs, so we ignore this subsystem entirely.
> > - In the patent's spec, the NLQ subsystem also loops over
> >   num_nlq_pivots, but even in the patent the number is hard-coded to one
> >   iteration rather than signalled. So we only store one set of coefs.
> >
> > Heavily influenced by https://github.com/quietvoid/dovi_tool
> > Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001
> >
> > Signed-off-by: Niklas Haas <git@haasn.dev>
> > ---
> >  configure             |   2 +
> >  libavcodec/Makefile   |   1 +
> >  libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
> >  libavcodec/dovi_rpu.h |  71 +++++++
> >  4 files changed, 504 insertions(+)
> >  create mode 100644 libavcodec/dovi_rpu.c
> >  create mode 100644 libavcodec/dovi_rpu.h
> >
> > diff --git a/configure b/configure
> > index 0ccd3bda11..68658a847f 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2434,6 +2434,7 @@ CONFIG_EXTRA="
> >      cbs_vp9
> >      dirac_parse
> >      dnn
> > +    dovi_rpu
> >      dvprofile
> >      exif
> >      faandct
> > @@ -2706,6 +2707,7 @@ cbs_mpeg2_select="cbs"
> >  cbs_vp9_select="cbs"
> >  dct_select="rdft"
> >  dirac_parse_select="golomb"
> > +dovi_rpu_select="golomb"
> >  dnn_suggest="libtensorflow libopenvino"
> >  dnn_deps="avformat swscale"
> >  error_resilience_select="me_cmp"
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index fb90ecea84..7364c7a91f 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -77,6 +77,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
> >  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> >  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
> >  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o
> dct32_float.o
> > +OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
> >  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
> >  OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
> >  OBJS-$(CONFIG_FAANDCT)                 += faandct.o
> > diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
> > new file mode 100644
> > index 0000000000..fc2e1fb2a1
> > --- /dev/null
> > +++ b/libavcodec/dovi_rpu.c
> > @@ -0,0 +1,430 @@
> > +/*
> > + * 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
> > + */
> > +
> > +#include "libavutil/buffer.h"
> > +
> > +#include "dovi_rpu.h"
> > +#include "golomb.h"
> > +#include "get_bits.h"
> > +
> > +enum {
> > +    RPU_COEFF_FIXED = 0,
> > +    RPU_COEFF_FLOAT = 1,
> > +};
> > +
> > +/**
> > + * Private contents of vdr_ref.
> > + */
> > +typedef struct DOVIVdrRef {
> > +    AVDOVIDataMapping mapping;
> > +    AVDOVIColorMetadata color;
> > +} DOVIVdrRef;
> > +
> > +void ff_dovi_ctx_unref(DOVIContext *s)
> > +{
> > +    for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr_ref); i++)
> > +        av_buffer_unref(&s->vdr_ref[i]);
> > +
> > +    /* Preserve the user-provided fields explicitly, reset everything
> else */
> > +    *s = (DOVIContext) {
> > +        .logctx = s->logctx,
> > +        .config = s->config,
> > +    };
> > +}
> > +
> > +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
> > +{
> > +    int ret;
> > +    s->logctx = s0->logctx;
> > +    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:
> > +    ff_dovi_ctx_unref(s);
> > +    return ret;
> > +}
> > +
> > +int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame)
> > +{
> > +    AVFrameSideData *sd;
> > +    AVBufferRef *buf;
> > +    AVDOVIMetadata *dovi;
> > +    size_t dovi_size;
> > +
> > +    if (!s->mapping || !s->color)
> > +        return 0; /* incomplete dovi metadata */
> > +
> > +    dovi = av_dovi_metadata_alloc(&dovi_size);
> > +    if (!dovi)
> > +        return AVERROR(ENOMEM);
> > +
> > +    buf = av_buffer_create((uint8_t *) dovi, dovi_size, NULL, NULL, 0);
> > +    if (!buf) {
> > +        av_free(dovi);
> > +        return AVERROR(ENOMEM);
> > +    }
> > +
> > +    sd = av_frame_new_side_data_from_buf(frame,
> AV_FRAME_DATA_DOVI_METADATA, buf);
> > +    if (!sd) {
> > +        av_buffer_unref(&buf);
> > +        return AVERROR(ENOMEM);
> > +    }
> > +
> > +    memcpy(av_dovi_get_header(dovi), &s->header, sizeof(s->header));
> > +    memcpy(av_dovi_get_mapping(dovi), s->mapping, sizeof(*s->mapping));
> > +    memcpy(av_dovi_get_color(dovi), s->color, sizeof(*s->color));
>
> These are potentially problematic due to trailing padding in the
> structures. This trailing padding might become valid fields in newer
> versions of this structure. E.g. av_dovi_metadata_alloc() might
> intentionally set the default values of these new fields to invalid
> values. Yet lavc (with the old structure) sees them as padding, allowing
> the compiler to trash these values on every write to these structures.
> More likely though is that they will never be touched at all and
> therefore retain their default (zero) initialization. Which might
> conflict with the default value of these fields.
> This can be fixed by only copying everything up to and including the
> last field that we know of for every of these structures. This will
> unfortunately increase the maintainence burden a bit.
>

This code is the primary source of any data in this struct. If it ever gets
extended, this parser wanting to store more fields would be why, so
initialization doesn't seem overly worrysome.


> > +    return 0;
> > +}
> > +
> > +static int guess_profile(const AVDOVIRpuDataHeader *hdr)
> > +{
> > +    switch (hdr->vdr_rpu_profile) {
> > +    case 0:
> > +        if (hdr->bl_video_full_range_flag)
> > +            return 5;
> > +        break;
> > +    case 1:
> > +        if (hdr->el_spatial_resampling_filter_flag &&
> !hdr->disable_residual_flag) {
> > +            if (hdr->vdr_bit_depth == 12) {
> > +                return 7;
> > +            } else {
> > +                return 4;
> > +            }
> > +        } else {
> > +            return 8;
> > +        }
> > +    }
> > +
> > +    return 0; /* unknown */
> > +}
> > +
> > +static inline uint64_t get_ue_coef(GetBitContext *gb, const
> AVDOVIRpuDataHeader *hdr)
> > +{
> > +    uint64_t ipart;
> > +    union { uint32_t u32; float f32; } fpart;
> > +
> > +    switch (hdr->coef_data_type) {
> > +    case RPU_COEFF_FIXED:
> > +        ipart = get_ue_golomb_long(gb);
> > +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> > +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
>
> Here and in get_se_coef below the f32 member of the union is not used
> with RPU_COEFF_FIXED, so using the union seems unnecessary. It is also
> slightly confusing, because upon seeing this and seeing that the union
> is used with RPU_COEFF_FIXED, I expect it to be used for type-punning.
>
> > +
> > +    case RPU_COEFF_FLOAT:
> > +        fpart.u32 = get_bits_long(gb, 32);
> > +        return fpart.f32 * (1 << hdr->coef_log2_denom);
>
> You could just use av_int2float() here instead of adding the union
> yourself.
>
> > +    }
> > +
> > +    return 0; /* unreachable */
> > +}
> > +
> > +static inline int64_t get_se_coef(GetBitContext *gb, const
> AVDOVIRpuDataHeader *hdr)
> > +{
> > +    int64_t ipart;
> > +    union { uint32_t u32; float f32; } fpart;
> > +
> > +    switch (hdr->coef_data_type) {
> > +    case RPU_COEFF_FIXED:
> > +        ipart = get_se_golomb_long(gb);
> > +        fpart.u32 = get_bits_long(gb, hdr->coef_log2_denom);
> > +        return (ipart << hdr->coef_log2_denom) + fpart.u32;
> > +
> > +    case RPU_COEFF_FLOAT:
> > +        fpart.u32 = get_bits_long(gb, 32);
> > +        return fpart.f32 * (1 << hdr->coef_log2_denom);
> > +    }
> > +
> > +    return 0; /* unreachable */
> > +}
> > +
> > +#define VALIDATE(VAR, MIN, MAX)
>          \
> > +    do {
>         \
> > +        if (VAR < MIN || VAR > MAX) {
>          \
> > +            av_log(s->logctx, AV_LOG_ERROR, "RPU validation failed: "
>          \
> > +                   #MIN" <= "#VAR" = %d <= "#MAX"\n", (int) VAR);
>          \
> > +            goto fail;
>         \
> > +        }
>          \
> > +    } while (0)
> > +
> > +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t
> rpu_size)
> > +{
> > +    AVDOVIRpuDataHeader *hdr = &s->header;
> > +    GetBitContext *gb = &(GetBitContext){0};
> > +    DOVIVdrRef *vdr;
> > +    int ret;
> > +
> > +    uint8_t nal_prefix;
> > +    uint8_t rpu_type;
> > +    uint8_t vdr_seq_info_present;
> > +    uint8_t vdr_dm_metadata_present;
> > +    uint8_t use_prev_vdr_rpu;
> > +    uint8_t use_nlq;
> > +    uint8_t profile;
> > +    if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0)
>
> Notice that rpu_size is converted to int before init_get_bits8() is
> called. (It is not problematic with the use in 6/6 because the size has
> already been checked in this case.)
>
> > +        return ret;
> > +
> _______________________________________________
> 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".
>

Hendrik Leppkes:
> Andreas Rheinhardt <andreas.rheinhardt@outlook.com> schrieb am So., 2. Jan.
> 2022, 05:18:
> 
>> Niklas Haas:
>>> From: Niklas Haas <git@haasn.dev>
>>>
>>> Based on a mixture of guesswork, partial documentation in patents, and
>>> reverse engineering of real-world samples. Confirmed working for all the
>>> samples I've thrown at it.
>>>
>>> Contains some annoying machinery to persist these values in between
>>> frames, which is needed in theory even though I've never actually seen a
>>> sample that relies on it in practice. May or may not work.
>>>
>>> Since the distinction matters greatly for parsing the color matrix
>>> values, this includes a small helper function to guess the right profile
>>> from the RPU itself in case the user has forgotten to forward the dovi
>>> configuration record to the decoder. (Which in practice, only ffmpeg.c
>>> and ffplay do..)
>>>
>>> Notable omissions / deviations:
>>> - CRC32 verification. This is based on the MPEG2 CRC32 type, which does
>>>   not seem to be implemented in lavu. (And I don't care enough to do so)
>>> - Linear interpolation support. Nothing documents this (beyond its
>>>   existence) and no samples use it, so impossible to implement.
>>> - All of the extension metadata blocks, but these contain values that
>>>   seem largely congruent with ST2094, HDR10, or other existing forms of
>>>   side data, so I will defer parsing/attaching them to a future commit.
>>> - The patent describes a mechanism for predicting coefficients from
>>>   previous RPUs, but the bit for the flag whether to use the
>>>   prediction deltas or signal entirely new coefficients does not seem to
>>>   be present in actual RPUs, so we ignore this subsystem entirely.
>>> - In the patent's spec, the NLQ subsystem also loops over
>>>   num_nlq_pivots, but even in the patent the number is hard-coded to one
>>>   iteration rather than signalled. So we only store one set of coefs.
>>>
>>> Heavily influenced by https://github.com/quietvoid/dovi_tool
>>> Documentation drawn from US Patent 10,701,399 B2 and ETSI GS CCM 001
>>>
>>> Signed-off-by: Niklas Haas <git@haasn.dev>
>>> ---
>>>  configure             |   2 +
>>>  libavcodec/Makefile   |   1 +
>>>  libavcodec/dovi_rpu.c | 430 ++++++++++++++++++++++++++++++++++++++++++
>>>  libavcodec/dovi_rpu.h |  71 +++++++
>>>  4 files changed, 504 insertions(+)
>>>  create mode 100644 libavcodec/dovi_rpu.c
>>>  create mode 100644 libavcodec/dovi_rpu.h
>>>
>>> diff --git a/configure b/configure
>>> index 0ccd3bda11..68658a847f 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -2434,6 +2434,7 @@ CONFIG_EXTRA="
>>>      cbs_vp9
>>>      dirac_parse
>>>      dnn
>>> +    dovi_rpu
>>>      dvprofile
>>>      exif
>>>      faandct
>>> @@ -2706,6 +2707,7 @@ cbs_mpeg2_select="cbs"
>>>  cbs_vp9_select="cbs"
>>>  dct_select="rdft"
>>>  dirac_parse_select="golomb"
>>> +dovi_rpu_select="golomb"
>>>  dnn_suggest="libtensorflow libopenvino"
>>>  dnn_deps="avformat swscale"
>>>  error_resilience_select="me_cmp"
>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
>>> index fb90ecea84..7364c7a91f 100644
>>> --- a/libavcodec/Makefile
>>> +++ b/libavcodec/Makefile
>>> @@ -77,6 +77,7 @@ OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>>>  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
>>>  OBJS-$(CONFIG_CRYSTALHD)               += crystalhd.o
>>>  OBJS-$(CONFIG_DCT)                     += dct.o dct32_fixed.o
>> dct32_float.o
>>> +OBJS-$(CONFIG_DOVI_RPU)                += dovi_rpu.o
>>>  OBJS-$(CONFIG_ERROR_RESILIENCE)        += error_resilience.o
>>>  OBJS-$(CONFIG_EXIF)                    += exif.o tiff_common.o
>>>  OBJS-$(CONFIG_FAANDCT)                 += faandct.o
>>> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
>>> new file mode 100644
>>> index 0000000000..fc2e1fb2a1
>>> --- /dev/null
>>> +++ b/libavcodec/dovi_rpu.c
>>> @@ -0,0 +1,430 @@
>>> +/*
>>> + * 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
>>> + */
>>> +
>>> +#include "libavutil/buffer.h"
>>> +
>>> +#include "dovi_rpu.h"
>>> +#include "golomb.h"
>>> +#include "get_bits.h"
>>> +
>>> +enum {
>>> +    RPU_COEFF_FIXED = 0,
>>> +    RPU_COEFF_FLOAT = 1,
>>> +};
>>> +
>>> +/**
>>> + * Private contents of vdr_ref.
>>> + */
>>> +typedef struct DOVIVdrRef {
>>> +    AVDOVIDataMapping mapping;
>>> +    AVDOVIColorMetadata color;
>>> +} DOVIVdrRef;
>>> +
>>> +void ff_dovi_ctx_unref(DOVIContext *s)
>>> +{
>>> +    for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr_ref); i++)
>>> +        av_buffer_unref(&s->vdr_ref[i]);
>>> +
>>> +    /* Preserve the user-provided fields explicitly, reset everything
>> else */
>>> +    *s = (DOVIContext) {
>>> +        .logctx = s->logctx,
>>> +        .config = s->config,
>>> +    };
>>> +}
>>> +
>>> +int ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
>>> +{
>>> +    int ret;
>>> +    s->logctx = s0->logctx;
>>> +    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:
>>> +    ff_dovi_ctx_unref(s);
>>> +    return ret;
>>> +}
>>> +
>>> +int ff_dovi_attach_side_data(DOVIContext *s, AVFrame *frame)
>>> +{
>>> +    AVFrameSideData *sd;
>>> +    AVBufferRef *buf;
>>> +    AVDOVIMetadata *dovi;
>>> +    size_t dovi_size;
>>> +
>>> +    if (!s->mapping || !s->color)
>>> +        return 0; /* incomplete dovi metadata */
>>> +
>>> +    dovi = av_dovi_metadata_alloc(&dovi_size);
>>> +    if (!dovi)
>>> +        return AVERROR(ENOMEM);
>>> +
>>> +    buf = av_buffer_create((uint8_t *) dovi, dovi_size, NULL, NULL, 0);
>>> +    if (!buf) {
>>> +        av_free(dovi);
>>> +        return AVERROR(ENOMEM);
>>> +    }
>>> +
>>> +    sd = av_frame_new_side_data_from_buf(frame,
>> AV_FRAME_DATA_DOVI_METADATA, buf);
>>> +    if (!sd) {
>>> +        av_buffer_unref(&buf);
>>> +        return AVERROR(ENOMEM);
>>> +    }
>>> +
>>> +    memcpy(av_dovi_get_header(dovi), &s->header, sizeof(s->header));
>>> +    memcpy(av_dovi_get_mapping(dovi), s->mapping, sizeof(*s->mapping));
>>> +    memcpy(av_dovi_get_color(dovi), s->color, sizeof(*s->color));
>>
>> These are potentially problematic due to trailing padding in the
>> structures. This trailing padding might become valid fields in newer
>> versions of this structure. E.g. av_dovi_metadata_alloc() might
>> intentionally set the default values of these new fields to invalid
>> values. Yet lavc (with the old structure) sees them as padding, allowing
>> the compiler to trash these values on every write to these structures.
>> More likely though is that they will never be touched at all and
>> therefore retain their default (zero) initialization. Which might
>> conflict with the default value of these fields.
>> This can be fixed by only copying everything up to and including the
>> last field that we know of for every of these structures. This will
>> unfortunately increase the maintainence burden a bit.
>>
> 
> This code is the primary source of any data in this struct. If it ever gets
> extended, this parser wanting to store more fields would be why, so
> initialization doesn't seem overly worrysome.
> 

It is indeed reasonable to expect that any extension to any of these
structs would be necessitated by this code and patches to this code
making use of these new fields will be part of the same patchset adding
the new fields. But even then it is still possible to use a new lavu
with an old lavc in which case the above mentioned problem exists.
This scenario could be precluded by forbidding mixing libraries from
different snapshots.

- Andreas