diff mbox series

[FFmpeg-devel,v2,3/3] libdav1d: use film grain export flag to export AVFilmGrainParams side data

Message ID MLxY3Ix--3-2@lynne.ee
State Accepted
Headers show
Series [FFmpeg-devel,v2,1/3] libavutil: introduce AVFilmGrainParams side data | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Lynne Nov. 12, 2020, 5:42 p.m. UTC 
This patch is relatively straightforward with one exception: 
the decoder option flag.
The option was introduced to troubleshoot but its existence is conflicting
and redundant now that we have a codec-generic flag.
Hence this patch deprecates it.

The way it interacts with AV_CODEC_EXPORT_DATA_FILM_GRAIN is as follows:

If filmgrain is unset and AV_CODEC_EXPORT_DATA_FILM_GRAIN is
present, disable film grain application and export side data.

If filmgrain is set to 0, disable film grain and export side data.

If filmgrain is set to 1, apply film grain but export side data if
the AV_CODEC_EXPORT_DATA_FILM_GRAIN flag is set. This may result in
double film grain application, but the user has requested it by setting
both.

Patch attached.
Subject: [PATCH v2 3/3] libdav1d: use film grain export flag to export
 AVFilmGrainParams side data

This patch is relatively straightforward with one exception:
the decoder option flag.
The option was introduced to troubleshoot but its existence is conflicting
and redundant now that we have a codec-generic flag.
Hence this patch deprecates it.

The way it interacts with AV_CODEC_EXPORT_DATA_FILM_GRAIN is as follows:

If filmgrain is unset and AV_CODEC_EXPORT_DATA_FILM_GRAIN is
present, disable film grain application and export side data.

If filmgrain is set to 0, disable film grain and export side data.

If filmgrain is set to 1, apply film grain but export side data if
the AV_CODEC_EXPORT_DATA_FILM_GRAIN flag is set. This may result in
double film grain application, but the user has requested it by setting
both.
---
 libavcodec/libdav1d.c | 82 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

Comments

James Almer Nov. 12, 2020, 6:07 p.m. UTC | #1 
On 11/12/2020 2:42 PM, Lynne wrote:
> This patch is relatively straightforward with one exception:
> the decoder option flag.
> The option was introduced to troubleshoot but its existence is conflicting
> and redundant now that we have a codec-generic flag.
> Hence this patch deprecates it.
> 
> The way it interacts with AV_CODEC_EXPORT_DATA_FILM_GRAIN is as follows:
> 
> If filmgrain is unset and AV_CODEC_EXPORT_DATA_FILM_GRAIN is
> present, disable film grain application and export side data.
> 
> If filmgrain is set to 0, disable film grain and export side data.
> 
> If filmgrain is set to 1, apply film grain but export side data if
> the AV_CODEC_EXPORT_DATA_FILM_GRAIN flag is set. This may result in
> double film grain application, but the user has requested it by setting
> both.
> 
> Patch attached.

> From c919abe5749e2ead9e84b03496d0fcec27a2f27e Mon Sep 17 00:00:00 2001
> From: Lynne <dev@lynne.ee>
> Date: Thu, 12 Nov 2020 12:48:20 +0100
> Subject: [PATCH v2 3/3] libdav1d: use film grain export flag to export
>  AVFilmGrainParams side data
> 
> This patch is relatively straightforward with one exception:
> the decoder option flag.
> The option was introduced to troubleshoot but its existence is conflicting
> and redundant now that we have a codec-generic flag.
> Hence this patch deprecates it.
> 
> The way it interacts with AV_CODEC_EXPORT_DATA_FILM_GRAIN is as follows:
> 
> If filmgrain is unset and AV_CODEC_EXPORT_DATA_FILM_GRAIN is
> present, disable film grain application and export side data.
> 
> If filmgrain is set to 0, disable film grain and export side data.
> 
> If filmgrain is set to 1, apply film grain but export side data if
> the AV_CODEC_EXPORT_DATA_FILM_GRAIN flag is set. This may result in
> double film grain application, but the user has requested it by setting
> both.
> ---
>  libavcodec/libdav1d.c | 82 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
> index 3af7ef4edc..319a9f6b47 100644
> --- a/libavcodec/libdav1d.c
> +++ b/libavcodec/libdav1d.c
> @@ -22,6 +22,7 @@
>  #include <dav1d/dav1d.h>
>  
>  #include "libavutil/avassert.h"
> +#include "libavutil/film_grain_params.h"
>  #include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/opt.h"
> @@ -37,6 +38,7 @@ typedef struct Libdav1dContext {
>      Dav1dContext *c;
>      AVBufferPool *pool;
>      int pool_size;
> +    AVBufferRef *film_grain_params;
>  
>      Dav1dData data;
>      int tile_threads;
> @@ -137,6 +139,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
>      s.frame_size_limit = c->max_pixels;
>      if (dav1d->apply_grain >= 0)
>          s.apply_grain = dav1d->apply_grain;
> +    else if (c->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN)
> +        s.apply_grain = 0;
>  
>      s.all_layers = dav1d->all_layers;
>      if (dav1d->operating_point >= 0)
> @@ -162,6 +166,7 @@ static void libdav1d_flush(AVCodecContext *c)
>  {
>      Libdav1dContext *dav1d = c->priv_data;
>  
> +    av_buffer_unref(&dav1d->film_grain_params);
>      dav1d_data_unref(&dav1d->data);
>      dav1d_flush(dav1d->c);
>  }
> @@ -395,6 +400,80 @@ FF_ENABLE_DEPRECATION_WARNINGS
>              break;
>          }
>      }
> +    if (p->frame_hdr->film_grain.present && (!dav1d->apply_grain ||
> +        (c->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN))) {
> +        AVFilmGrainParams *fgp;
> +        if (p->frame_hdr->film_grain.update) {

You're not gaining anything by looking at this field. You're either 
allocating a new buffer or making the existing one writable. There is no 
case where it can be reused for more than one frame, so you'll be always 
allocating stuff and copying data.

Also, libdav1d already takes care of ensuring p->frame_hdr->film_grain 
contains the correct values for every frame for us (It actually takes 
them from the correct reference frame pointed by 
film_grain_params_ref_idx when film_grain.update is false, whereas here 
you're always using the one from the previous returned frame), so just 
use av_film_grain_params_create_side_data() then copy the fields, like 
mastering and content level do above.

> +            size_t fgp_size;
> +            fgp = av_film_grain_params_alloc(&fgp_size);
> +            if (!fgp) {
> +                res = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +
> +            AVBufferRef *buf = av_buffer_create((uint8_t *)fgp, fgp_size,
> +                                                av_buffer_default_free, NULL, 0);
> +            if (!buf) {
> +                res = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +
> +            av_buffer_unref(&dav1d->film_grain_params);
> +            dav1d->film_grain_params = buf;
> +
> +            fgp->type = AV_FILM_GRAM_PARAMS_AV1;
> +            fgp->seed = p->frame_hdr->film_grain.data.seed;
> +            fgp->limit_output_range = p->frame_hdr->film_grain.data.clip_to_restricted_range;
> +            fgp->codec.aom.num_y_points = p->frame_hdr->film_grain.data.num_y_points;
> +            fgp->codec.aom.chroma_scaling_from_luma = p->frame_hdr->film_grain.data.chroma_scaling_from_luma;
> +            fgp->codec.aom.scaling_shift = p->frame_hdr->film_grain.data.scaling_shift;
> +            fgp->codec.aom.ar_coeff_lag = p->frame_hdr->film_grain.data.ar_coeff_lag;
> +            fgp->codec.aom.ar_coeff_shift = p->frame_hdr->film_grain.data.ar_coeff_shift;
> +            fgp->codec.aom.grain_scale_shift = p->frame_hdr->film_grain.data.grain_scale_shift;
> +            fgp->codec.aom.overlap_flag = p->frame_hdr->film_grain.data.overlap_flag;
> +
> +            memcpy(&fgp->codec.aom.y_points, &p->frame_hdr->film_grain.data.y_points,
> +                   sizeof(fgp->codec.aom.y_points));
> +            memcpy(&fgp->codec.aom.num_uv_points, &p->frame_hdr->film_grain.data.num_uv_points,
> +                   sizeof(fgp->codec.aom.num_uv_points));
> +            memcpy(&fgp->codec.aom.uv_points, &p->frame_hdr->film_grain.data.uv_points,
> +                   sizeof(fgp->codec.aom.uv_points));
> +            memcpy(&fgp->codec.aom.ar_coeffs_y, &p->frame_hdr->film_grain.data.ar_coeffs_y,
> +                   sizeof(fgp->codec.aom.ar_coeffs_y));
> +            memcpy(&fgp->codec.aom.ar_coeffs_uv, &p->frame_hdr->film_grain.data.ar_coeffs_uv,
> +                   sizeof(fgp->codec.aom.ar_coeffs_uv));
> +            memcpy(&fgp->codec.aom.uv_mult, &p->frame_hdr->film_grain.data.uv_mult,
> +                   sizeof(fgp->codec.aom.uv_mult));
> +            memcpy(&fgp->codec.aom.uv_mult_luma, &p->frame_hdr->film_grain.data.uv_luma_mult,
> +                   sizeof(fgp->codec.aom.uv_mult_luma));
> +            memcpy(&fgp->codec.aom.uv_offset, &p->frame_hdr->film_grain.data.uv_offset,
> +                   sizeof(fgp->codec.aom.uv_offset));
> +        } else if (dav1d->film_grain_params) {
> +            if (av_buffer_make_writable(&dav1d->film_grain_params) < 0) {
> +                res = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +
> +            /* The RNG seed is always updated even if update == 0 */
> +            fgp = (AVFilmGrainParams *)dav1d->film_grain_params->data;
> +            fgp->seed = p->frame_hdr->film_grain.data.seed;
> +        }
> +
> +        if (dav1d->film_grain_params) {
> +            AVBufferRef *fgp_ref = av_buffer_ref(dav1d->film_grain_params);
> +            if (!fgp_ref) {
> +                res = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +            if (!av_frame_new_side_data_from_buf(frame,
> +                                                 AV_FRAME_DATA_FILM_GRAIN_PARAMS,
> +                                                 fgp_ref)) {
> +                av_buffer_unref(&fgp_ref);
> +                res = AVERROR(ENOMEM);
> +                goto fail;
> +            }
> +        }
> +    }
>  
>      res = 0;
>  fail:
> @@ -408,6 +487,7 @@ static av_cold int libdav1d_close(AVCodecContext *c)
>  {
>      Libdav1dContext *dav1d = c->priv_data;
>  
> +    av_buffer_unref(&dav1d->film_grain_params);
>      av_buffer_pool_uninit(&dav1d->pool);
>      dav1d_data_unref(&dav1d->data);
>      dav1d_close(&dav1d->c);
> @@ -420,7 +500,7 @@ static av_cold int libdav1d_close(AVCodecContext *c)
>  static const AVOption libdav1d_options[] = {
>      { "tilethreads", "Tile threads", OFFSET(tile_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
>      { "framethreads", "Frame threads", OFFSET(frame_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
> -    { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
> +    { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD | AV_OPT_FLAG_DEPRECATED },
>      { "oppoint",  "Select an operating point of the scalable bitstream", OFFSET(operating_point), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 31, VD },
>      { "alllayers", "Output all spatial layers", OFFSET(all_layers), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
>      { NULL }
> -- 
> 2.29.2
>
Lynne Nov. 12, 2020, 6:44 p.m. UTC | #2 
Nov 12, 2020, 19:07 by jamrial@gmail.com:

> On 11/12/2020 2:42 PM, Lynne wrote:
>
>> +    if (p->frame_hdr->film_grain.present && (!dav1d->apply_grain ||
>> +        (c->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN))) {
>> +        AVFilmGrainParams *fgp;
>> +        if (p->frame_hdr->film_grain.update) {
>>
>
> You're not gaining anything by looking at this field. You're either allocating a new buffer or making the existing one writable. There is no case where it can be reused for more than one frame, so you'll be always allocating stuff and copying data.
>
> Also, libdav1d already takes care of ensuring p->frame_hdr->film_grain contains the correct values for every frame for us (It actually takes them from the correct reference frame pointed by film_grain_params_ref_idx when film_grain.update is false, whereas here you're always using the one from the previous returned frame), so just use av_film_grain_params_create_side_data() then copy the fields, like mastering and content level do above.
>

Fair enough. Still saved a copy occasionally.
v3 attached, does the same as all other side data.
diff mbox series

Patch

diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
index 3af7ef4edc..319a9f6b47 100644
--- a/libavcodec/libdav1d.c
+++ b/libavcodec/libdav1d.c
@@ -22,6 +22,7 @@ 
 #include <dav1d/dav1d.h>
 
 #include "libavutil/avassert.h"
+#include "libavutil/film_grain_params.h"
 #include "libavutil/mastering_display_metadata.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/opt.h"
@@ -37,6 +38,7 @@  typedef struct Libdav1dContext {
     Dav1dContext *c;
     AVBufferPool *pool;
     int pool_size;
+    AVBufferRef *film_grain_params;
 
     Dav1dData data;
     int tile_threads;
@@ -137,6 +139,8 @@  static av_cold int libdav1d_init(AVCodecContext *c)
     s.frame_size_limit = c->max_pixels;
     if (dav1d->apply_grain >= 0)
         s.apply_grain = dav1d->apply_grain;
+    else if (c->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN)
+        s.apply_grain = 0;
 
     s.all_layers = dav1d->all_layers;
     if (dav1d->operating_point >= 0)
@@ -162,6 +166,7 @@  static void libdav1d_flush(AVCodecContext *c)
 {
     Libdav1dContext *dav1d = c->priv_data;
 
+    av_buffer_unref(&dav1d->film_grain_params);
     dav1d_data_unref(&dav1d->data);
     dav1d_flush(dav1d->c);
 }
@@ -395,6 +400,80 @@  FF_ENABLE_DEPRECATION_WARNINGS
             break;
         }
     }
+    if (p->frame_hdr->film_grain.present && (!dav1d->apply_grain ||
+        (c->export_side_data & AV_CODEC_EXPORT_DATA_FILM_GRAIN))) {
+        AVFilmGrainParams *fgp;
+        if (p->frame_hdr->film_grain.update) {
+            size_t fgp_size;
+            fgp = av_film_grain_params_alloc(&fgp_size);
+            if (!fgp) {
+                res = AVERROR(ENOMEM);
+                goto fail;
+            }
+
+            AVBufferRef *buf = av_buffer_create((uint8_t *)fgp, fgp_size,
+                                                av_buffer_default_free, NULL, 0);
+            if (!buf) {
+                res = AVERROR(ENOMEM);
+                goto fail;
+            }
+
+            av_buffer_unref(&dav1d->film_grain_params);
+            dav1d->film_grain_params = buf;
+
+            fgp->type = AV_FILM_GRAM_PARAMS_AV1;
+            fgp->seed = p->frame_hdr->film_grain.data.seed;
+            fgp->limit_output_range = p->frame_hdr->film_grain.data.clip_to_restricted_range;
+            fgp->codec.aom.num_y_points = p->frame_hdr->film_grain.data.num_y_points;
+            fgp->codec.aom.chroma_scaling_from_luma = p->frame_hdr->film_grain.data.chroma_scaling_from_luma;
+            fgp->codec.aom.scaling_shift = p->frame_hdr->film_grain.data.scaling_shift;
+            fgp->codec.aom.ar_coeff_lag = p->frame_hdr->film_grain.data.ar_coeff_lag;
+            fgp->codec.aom.ar_coeff_shift = p->frame_hdr->film_grain.data.ar_coeff_shift;
+            fgp->codec.aom.grain_scale_shift = p->frame_hdr->film_grain.data.grain_scale_shift;
+            fgp->codec.aom.overlap_flag = p->frame_hdr->film_grain.data.overlap_flag;
+
+            memcpy(&fgp->codec.aom.y_points, &p->frame_hdr->film_grain.data.y_points,
+                   sizeof(fgp->codec.aom.y_points));
+            memcpy(&fgp->codec.aom.num_uv_points, &p->frame_hdr->film_grain.data.num_uv_points,
+                   sizeof(fgp->codec.aom.num_uv_points));
+            memcpy(&fgp->codec.aom.uv_points, &p->frame_hdr->film_grain.data.uv_points,
+                   sizeof(fgp->codec.aom.uv_points));
+            memcpy(&fgp->codec.aom.ar_coeffs_y, &p->frame_hdr->film_grain.data.ar_coeffs_y,
+                   sizeof(fgp->codec.aom.ar_coeffs_y));
+            memcpy(&fgp->codec.aom.ar_coeffs_uv, &p->frame_hdr->film_grain.data.ar_coeffs_uv,
+                   sizeof(fgp->codec.aom.ar_coeffs_uv));
+            memcpy(&fgp->codec.aom.uv_mult, &p->frame_hdr->film_grain.data.uv_mult,
+                   sizeof(fgp->codec.aom.uv_mult));
+            memcpy(&fgp->codec.aom.uv_mult_luma, &p->frame_hdr->film_grain.data.uv_luma_mult,
+                   sizeof(fgp->codec.aom.uv_mult_luma));
+            memcpy(&fgp->codec.aom.uv_offset, &p->frame_hdr->film_grain.data.uv_offset,
+                   sizeof(fgp->codec.aom.uv_offset));
+        } else if (dav1d->film_grain_params) {
+            if (av_buffer_make_writable(&dav1d->film_grain_params) < 0) {
+                res = AVERROR(ENOMEM);
+                goto fail;
+            }
+
+            /* The RNG seed is always updated even if update == 0 */
+            fgp = (AVFilmGrainParams *)dav1d->film_grain_params->data;
+            fgp->seed = p->frame_hdr->film_grain.data.seed;
+        }
+
+        if (dav1d->film_grain_params) {
+            AVBufferRef *fgp_ref = av_buffer_ref(dav1d->film_grain_params);
+            if (!fgp_ref) {
+                res = AVERROR(ENOMEM);
+                goto fail;
+            }
+            if (!av_frame_new_side_data_from_buf(frame,
+                                                 AV_FRAME_DATA_FILM_GRAIN_PARAMS,
+                                                 fgp_ref)) {
+                av_buffer_unref(&fgp_ref);
+                res = AVERROR(ENOMEM);
+                goto fail;
+            }
+        }
+    }
 
     res = 0;
 fail:
@@ -408,6 +487,7 @@  static av_cold int libdav1d_close(AVCodecContext *c)
 {
     Libdav1dContext *dav1d = c->priv_data;
 
+    av_buffer_unref(&dav1d->film_grain_params);
     av_buffer_pool_uninit(&dav1d->pool);
     dav1d_data_unref(&dav1d->data);
     dav1d_close(&dav1d->c);
@@ -420,7 +500,7 @@  static av_cold int libdav1d_close(AVCodecContext *c)
 static const AVOption libdav1d_options[] = {
     { "tilethreads", "Tile threads", OFFSET(tile_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
     { "framethreads", "Frame threads", OFFSET(frame_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
-    { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
+    { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD | AV_OPT_FLAG_DEPRECATED },
     { "oppoint",  "Select an operating point of the scalable bitstream", OFFSET(operating_point), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 31, VD },
     { "alllayers", "Output all spatial layers", OFFSET(all_layers), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
     { NULL }