diff mbox series

[FFmpeg-devel,3/8] avcodec/dovi_rpuenc: try to re-use existing vdr_rpu_id

Message ID 20240618194221.26073-4-ffmpeg@haasn.xyz
State New
Headers show
Series DoVi metadata compression | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Niklas Haas June 18, 2024, 7:35 p.m. UTC 
From: Niklas Haas <git@haasn.dev>

And only override it if we either have an exact match, or if we still
have unused metadata slots (to avoid an overwrite).
---
 libavcodec/dovi_rpuenc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Niklas Haas June 18, 2024, 7:50 p.m. UTC | #1 
On Tue, 18 Jun 2024 21:35:33 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> And only override it if we either have an exact match, or if we still
> have unused metadata slots (to avoid an overwrite).
> ---
>  libavcodec/dovi_rpuenc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
> index 45fcd9a86c..0d49a128fd 100644
> --- a/libavcodec/dovi_rpuenc.c
> +++ b/libavcodec/dovi_rpuenc.c
> @@ -463,12 +463,12 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    vdr_rpu_id = -1;
> +    vdr_rpu_id = mapping->vdr_rpu_id;
>      for (int i = 0; i <= DOVI_MAX_DM_ID; i++) {
>          if (s->vdr[i] && !memcmp(s->vdr[i], mapping, sizeof(*mapping))) {
>              vdr_rpu_id = i;
>              break;
> -        } else if (vdr_rpu_id < 0 && (!s->vdr[i] || i == DOVI_MAX_DM_ID)) {
> +        } else if (s->vdr[vdr_rpu_id] && !s->vdr[i]) {
>              vdr_rpu_id = i;
>          }
>      }
> -- 
> 2.45.1
> 

I just noticed this check is wrong either way, because the `vdr_rpu_id`
is also included as part of `mapping`. If we reshuffle VDR IDs, we also
need to change the contents of `mapping` when updating the vdrs, as well
as omitting it when doing the `memcmp`.

Effectively this loop currently does nothing, since the only field that
can succeed is in the case `i == mapping->vdr_rpu_id`.

Will fix for v2.
diff mbox series

Patch

diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 45fcd9a86c..0d49a128fd 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -463,12 +463,12 @@  int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
         return AVERROR_INVALIDDATA;
     }
 
-    vdr_rpu_id = -1;
+    vdr_rpu_id = mapping->vdr_rpu_id;
     for (int i = 0; i <= DOVI_MAX_DM_ID; i++) {
         if (s->vdr[i] && !memcmp(s->vdr[i], mapping, sizeof(*mapping))) {
             vdr_rpu_id = i;
             break;
-        } else if (vdr_rpu_id < 0 && (!s->vdr[i] || i == DOVI_MAX_DM_ID)) {
+        } else if (s->vdr[vdr_rpu_id] && !s->vdr[i]) {
             vdr_rpu_id = i;
         }
     }