util/libav: do not bail on MEL-only RPUs w/EL

[intelfx]

We can still map DV metadata that indicates enhancement layer usage (!disable_residual_flag, Profile 7 and 4) for MEL-only bitstreams, which is indicated by trivial NLQ parameters.

[kasper93]

pl_map_avframe_ex() should also be fixed in similar fashion to use pl_can_map_avdovi_metadata(). And for parity with libdovi code path max_pq and avg_pq should be mapped also when dovi meta is not used. Also needs API version bump.

diff --git a/src/include/libplacebo/utils/libav_internal.h b/src/include/libplacebo/utils/libav_internal.h
index a754e98b..928d37f6 100644
--- a/src/include/libplacebo/utils/libav_internal.h
+++ b/src/include/libplacebo/utils/libav_internal.h
@@ -925,14 +925,14 @@ PL_LIBAV_API void pl_map_avdovi_metadata(struct pl_color_space *color,
             pl_hdr_rescale(PL_HDR_PQ, PL_HDR_NITS, dovi_color->source_min_pq / 4095.0f);
         color->hdr.max_luma =
             pl_hdr_rescale(PL_HDR_PQ, PL_HDR_NITS, dovi_color->source_max_pq / 4095.0f);
+    }

 #if LIBAVUTIL_VERSION_INT >= AV_VERSION_INT(59, 12, 100)
-        if ((dovi_ext = av_dovi_find_level(metadata, 1))) {
-            color->hdr.max_pq_y = dovi_ext->l1.max_pq / 4095.0f;
-            color->hdr.avg_pq_y = dovi_ext->l1.avg_pq / 4095.0f;
-        }
-#endif
+    if ((dovi_ext = av_dovi_find_level(metadata, 1))) {
+        color->hdr.max_pq_y = dovi_ext->l1.max_pq / 4095.0f;
+        color->hdr.avg_pq_y = dovi_ext->l1.avg_pq / 4095.0f;
     }
+#endif
 }

 PL_LIBAV_API void pl_frame_map_avdovi_metadata(struct pl_frame *out_frame,

which makes it awkward with dovi allocation part, but since the API is changed already it can be done while at it. See how pl_map_avframe_ex() maps RPU info always, regardless of header->disable_residual_flag.

[ValeZAA]

Any progress on this issue? Also any progress on NLQ and full FEL implementation? DoviBaker is good now.

[kasper93]

After thinking about this. I think the current PR is ok, but we should also add an option to allow using RPU metadata forcefully.

I propose to:

Remove pl_can_map_avdovi_metadata(metadata) check from pl_map_avdovi_metadata() and let caller to decide if they want to map dovi or not.

For example caller code could be then written such that, it allows external option to decide whether to map metadata.

        if (opts->force_dovi_rpu || pl_can_map_avdovi_metadata(metadata)) {
            pl_map_avdovi_metadata(&dst->params.color, &dst->params.repr, (void *)dst->dovi->data, metadata);

EDIT:

Also pl_can_map_avdovi_metadata may have better name, maybe pl_dovi_metadata_supported.

[intelfx]

I have split the suggested changes into their own commits just for visibility. I'll squash them back eventually. I'm not sure how to pass pl_options to pl_map_avframe_ex(); did I use the right options here or did you mean anything else?

I did bump the API version, but I'm not sure how I should handle the API semantics change. Do I declare a new function with new semantics and deprecate the old one, or should I leave it as it is now?

I also did not change the semantics of the already-deprecated pl_frame_map_avdovi_metadata().

[kasper93]

I have split the suggested changes into their own commits just for visibility. I'll squash them back eventually. I'm not sure how to pass pl_options to pl_map_avframe_ex(); did I use the right options here or did you mean anything else?

I think using params is fine.

I did bump the API version, but I'm not sure how I should handle the API semantics change. Do I declare a new function with new semantics and deprecate the old one, or should I leave it as it is now?

Which functions would you deprecate?

We could deprecate libdovi usage.

I also did not change the semantics of the already-deprecated pl_frame_map_avdovi_metadata().

That's fine.

I think the patch is fine, it will change mapping for users that directly map dovi metadata using those API, but we have api version bump and generally preserving some backward compatibility with not mapping this metadata, would require duplicating APIs. I don't think it's worth it.

/cc @haasn

[intelfx]

Which functions would you deprecate?

I meant deprecate the current pl_map_avdovi_metadata() (leaving its behavior intact) and add a new pl_map_avdovi_metadata2() or so with the checks lifted out of it. I.e. exactly what you are saying with "duplicating APIs". So okay, I'll skip that.

At least that's how they do it in projects with "perfect" API versioning and compat guarantees 🙃

[intelfx]

Updated the mpv counterpart in mpv-player/mpv#14692.

[haasn]

API-wise, I would offer a design like this:

PL_LIBAV_API bool pl_map_avdovi_metadata(..., const AVDOVIMetadata *metadata)

With the function returning whether successful. You can run it with color = repr = dovi = NULL to test for support without doing anything.

[kasper93]

I see the idea, but this would be awkward as it stands currently.

pl_map_avdovi_metadata() maps dovi regardless if it is fully supported or not. Meaning, it can be used to "force" part of dovi metadata (without FEL).

So it would do:

pl_map_avdovi_metadata(NULL, NULL, NULL, meta); => false (not fully supported)
pl_map_avdovi_metadata(&color, &repr, &dovi, meta); => true (mapped even if FEL)

Which is not incorrect, but little bit awkward to return false/true with the same meta.

It could return int or enum to be explicit about mapping state, but not sure if that's better?

[intelfx]

Is there anything for me to do here?

[haasn]

Should merge this in with the actual commits that change the API. (Even if it creates two separate API version bumps; though I would prefer to change all signatures in one go)

Edit: Nvm, I saw that you intend to squash these commits before merge