* [FFmpeg-devel] [PATCH 2/8] avcodec/dovi_rpu: properly handle vdr_dm_metadata_present
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 3/8] avcodec/dovi_rpu: fix dm_metadata_id handling Niklas Haas
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
When this is 0, the metadata is explicitly inferred to stated default
values from the spec, rather than inferred from the previous frame's
values.
Likewise, when encoding, instead of checking if the value changed since
the last frame, we need to check if it differs from the default.
---
libavcodec/dovi_rpu.c | 43 ++++++++++++++++++++++++++++++++++++++++
libavcodec/dovi_rpu.h | 3 +++
libavcodec/dovi_rpudec.c | 2 ++
libavcodec/dovi_rpuenc.c | 10 +++++-----
4 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
index b26c19dd5e..91134e031d 100644
--- a/libavcodec/dovi_rpu.c
+++ b/libavcodec/dovi_rpu.c
@@ -86,3 +86,46 @@ int ff_dovi_guess_profile_hevc(const AVDOVIRpuDataHeader *hdr)
return 0; /* unknown */
}
+
+const AVDOVIColorMetadata ff_dovi_color_default = {
+ .dm_metadata_id = 0,
+ .scene_refresh_flag = 0,
+ .ycc_to_rgb_matrix = {
+ { 9575, 8192 },
+ { 0, 8192 },
+ { 14742, 8192 },
+ { 9575, 8192 },
+ { 1754, 8192 },
+ { 4383, 8192 },
+ { 9575, 8192 },
+ { 17372, 8192 },
+ { 0, 8192 },
+ },
+ .ycc_to_rgb_offset = {
+ { 1, 4 },
+ { 2, 1 },
+ { 2, 1 },
+ },
+ .rgb_to_lms_matrix = {
+ { 5845, 16384 },
+ { 9702, 16384 },
+ { 837, 16384 },
+ { 2568, 16384 },
+ { 12256, 16384 },
+ { 1561, 16384 },
+ { 0, 16384 },
+ { 679, 16384 },
+ { 15705, 16384 },
+ },
+ .signal_eotf = 39322,
+ .signal_eotf_param0 = 15867,
+ .signal_eotf_param1 = 228,
+ .signal_eotf_param2 = 1383604,
+ .signal_bit_depth = 14,
+ .signal_color_space = 0,
+ .signal_chroma_format = 0,
+ .signal_full_range_flag = 1,
+ .source_min_pq = 62,
+ .source_max_pq = 3696,
+ .source_diagonal = 42,
+};
diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h
index 8ce0c88e9d..c784afbe4b 100644
--- a/libavcodec/dovi_rpu.h
+++ b/libavcodec/dovi_rpu.h
@@ -157,4 +157,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
*/
int ff_dovi_guess_profile_hevc(const AVDOVIRpuDataHeader *hdr);
+/* Default values for AVDOVIColorMetadata */
+extern const AVDOVIColorMetadata ff_dovi_color_default;
+
#endif /* AVCODEC_DOVI_RPU_H */
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index d1dcc3a262..cf2152988c 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -616,6 +616,8 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
color->source_min_pq = get_bits(gb, 12);
color->source_max_pq = get_bits(gb, 12);
color->source_diagonal = get_bits(gb, 10);
+ } else {
+ s->color = &ff_dovi_color_default;
}
/* Parse extension blocks */
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 3c3e0f84c0..242cd76c58 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -441,7 +441,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
const AVDOVIRpuDataHeader *hdr;
const AVDOVIDataMapping *mapping;
const AVDOVIColorMetadata *color;
- int vdr_dm_metadata_changed, vdr_rpu_id, use_prev_vdr_rpu, profile,
+ int vdr_dm_metadata_present, vdr_rpu_id, use_prev_vdr_rpu, profile,
buffer_size, rpu_size, pad, zero_run;
int num_ext_blocks_v1, num_ext_blocks_v2;
uint32_t crc;
@@ -512,7 +512,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
}
- vdr_dm_metadata_changed = !s->color || memcmp(s->color, color, sizeof(*color));
+ vdr_dm_metadata_present = memcmp(color, &ff_dovi_color_default, sizeof(*color));
use_prev_vdr_rpu = !memcmp(&s->vdr[vdr_rpu_id]->mapping, mapping, sizeof(*mapping));
buffer_size = 12 /* vdr seq info */ + 5 /* CRC32 + terminator */;
@@ -529,7 +529,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
}
}
- if (vdr_dm_metadata_changed)
+ if (vdr_dm_metadata_present)
buffer_size += 67;
av_fast_padded_malloc(&s->rpu_buf, &s->rpu_buf_sz, buffer_size);
@@ -560,7 +560,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
s->header = *hdr;
- put_bits(pb, 1, vdr_dm_metadata_changed);
+ put_bits(pb, 1, vdr_dm_metadata_present);
put_bits(pb, 1, use_prev_vdr_rpu);
set_ue_golomb(pb, vdr_rpu_id);
s->mapping = &s->vdr[vdr_rpu_id]->mapping;
@@ -632,7 +632,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
memcpy(&s->vdr[vdr_rpu_id]->mapping, mapping, sizeof(*mapping));
}
- if (vdr_dm_metadata_changed) {
+ if (vdr_dm_metadata_present) {
const int denom = profile == 4 ? (1 << 30) : (1 << 28);
set_ue_golomb(pb, color->dm_metadata_id); /* affected_dm_id */
set_ue_golomb(pb, color->dm_metadata_id); /* current_dm_id */
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 3/8] avcodec/dovi_rpu: fix dm_metadata_id handling
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 2/8] avcodec/dovi_rpu: properly handle vdr_dm_metadata_present Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 4/8] avcodec/dovi_rpudec: simplify vdr handling (cosmetic) Niklas Haas
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
Despite the suggestive size limits, this metadata ID has nothing to do
with the VDR metadata ID used for the data mappings. Actually, the
specification leaves them wholly unexplained, other than acknowleding
their existence. Must be some secret dolby sauce. They're not even
involved in DM metadata compression, which is handled using an entirely
separate ID.
That leaves us with a lack of anything sensible to do with these IDs.
Since we unfortunately only expose one `dm_metadata_id` field to the
user, just ensure that they match; which appears to always be the case
in practice. If somebody ever hits this error, I would really much
rather like to see the triggering file.
---
libavcodec/dovi_rpu.c | 3 +++
libavcodec/dovi_rpu.h | 1 +
libavcodec/dovi_rpudec.c | 30 +++++++++++++++---------------
libavcodec/dovi_rpuenc.c | 16 ++++++++--------
4 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c
index 91134e031d..5130a9598d 100644
--- a/libavcodec/dovi_rpu.c
+++ b/libavcodec/dovi_rpu.c
@@ -28,6 +28,7 @@
void ff_dovi_ctx_unref(DOVIContext *s)
{
+ ff_refstruct_unref(&s->dm);
for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr); i++)
ff_refstruct_unref(&s->vdr[i]);
ff_refstruct_unref(&s->ext_blocks);
@@ -40,6 +41,7 @@ void ff_dovi_ctx_unref(DOVIContext *s)
void ff_dovi_ctx_flush(DOVIContext *s)
{
+ ff_refstruct_unref(&s->dm);
for (int i = 0; i < FF_ARRAY_ELEMS(s->vdr); i++)
ff_refstruct_unref(&s->vdr[i]);
ff_refstruct_unref(&s->ext_blocks);
@@ -60,6 +62,7 @@ void ff_dovi_ctx_replace(DOVIContext *s, const DOVIContext *s0)
s->header = s0->header;
s->mapping = s0->mapping;
s->color = s0->color;
+ ff_refstruct_replace(&s->dm, s0->dm);
for (int i = 0; i <= DOVI_MAX_DM_ID; i++)
ff_refstruct_replace(&s->vdr[i], s0->vdr[i]);
ff_refstruct_replace(&s->ext_blocks, s0->ext_blocks);
diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h
index c784afbe4b..da9bd67cde 100644
--- a/libavcodec/dovi_rpu.h
+++ b/libavcodec/dovi_rpu.h
@@ -74,6 +74,7 @@ typedef struct DOVIContext {
/**
* Private fields internal to dovi_rpu.c
*/
+ AVDOVIColorMetadata *dm; ///< RefStruct
struct DOVIVdr *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references
uint8_t *rpu_buf; ///< temporary buffer
unsigned rpu_buf_sz;
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index cf2152988c..3e15453705 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -568,25 +568,25 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
int current_dm_id = get_ue_golomb_31(gb);
VALIDATE(affected_dm_id, 0, DOVI_MAX_DM_ID);
VALIDATE(current_dm_id, 0, DOVI_MAX_DM_ID);
- if (!s->vdr[affected_dm_id]) {
- s->vdr[affected_dm_id] = ff_refstruct_allocz(sizeof(DOVIVdr));
- if (!s->vdr[affected_dm_id])
- return AVERROR(ENOMEM);
+ if (affected_dm_id != current_dm_id) {
+ /* The spec does not explain these fields at all, and there is
+ * a lack of samples to understand how they're supposed to work,
+ * so just assert them being equal for now */
+ avpriv_request_sample(s->logctx, "affected/current_dm_metadata_id "
+ "mismatch? %u != %u\n", affected_dm_id, current_dm_id);
+ ff_dovi_ctx_unref(s);
+ return AVERROR_PATCHWELCOME;
}
- if (!s->vdr[current_dm_id]) {
- av_log(s->logctx, AV_LOG_ERROR, "Unknown previous RPU DM ID: %u\n",
- current_dm_id);
- goto fail;
+ if (!s->dm) {
+ s->dm = ff_refstruct_allocz(sizeof(AVDOVIColorMetadata));
+ if (!s->dm) {
+ ff_dovi_ctx_unref(s);
+ return AVERROR(ENOMEM);
+ }
}
- /* Update current pointer based on current_dm_id */
- vdr = s->vdr[current_dm_id];
- s->color = &vdr->color;
-
- /* Update values of affected_dm_id */
- vdr = s->vdr[affected_dm_id];
- color = &vdr->color;
+ s->color = color = s->dm;
color->dm_metadata_id = affected_dm_id;
color->scene_refresh_flag = get_ue_golomb_31(gb);
for (int i = 0; i < 9; i++)
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 242cd76c58..59e8ed6e3e 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -479,12 +479,6 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
return AVERROR(ENOMEM);
}
- if (!s->vdr[color->dm_metadata_id]) {
- s->vdr[color->dm_metadata_id] = ff_refstruct_allocz(sizeof(DOVIVdr));
- if (!s->vdr[color->dm_metadata_id])
- return AVERROR(ENOMEM);
- }
-
num_ext_blocks_v1 = num_ext_blocks_v2 = 0;
for (int i = 0; i < metadata->num_ext_blocks; i++) {
const AVDOVIDmData *dm = av_dovi_get_ext(metadata, i);
@@ -515,6 +509,12 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
vdr_dm_metadata_present = memcmp(color, &ff_dovi_color_default, sizeof(*color));
use_prev_vdr_rpu = !memcmp(&s->vdr[vdr_rpu_id]->mapping, mapping, sizeof(*mapping));
+ if (vdr_dm_metadata_present && !s->dm) {
+ s->dm = ff_refstruct_allocz(sizeof(AVDOVIColorMetadata));
+ if (!s->dm)
+ return AVERROR(ENOMEM);
+ }
+
buffer_size = 12 /* vdr seq info */ + 5 /* CRC32 + terminator */;
buffer_size += num_ext_blocks_v1 * 13;
buffer_size += num_ext_blocks_v2 * 28;
@@ -655,8 +655,8 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
put_bits(pb, 12, color->source_max_pq);
put_bits(pb, 10, color->source_diagonal);
- memcpy(&s->vdr[color->dm_metadata_id]->color, color, sizeof(*color));
- s->color = &s->vdr[color->dm_metadata_id]->color;
+ memcpy(s->dm, color, sizeof(*color));
+ s->color = s->dm;
}
set_ue_golomb(pb, num_ext_blocks_v1);
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 4/8] avcodec/dovi_rpudec: simplify vdr handling (cosmetic)
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 2/8] avcodec/dovi_rpu: properly handle vdr_dm_metadata_present Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 3/8] avcodec/dovi_rpu: fix dm_metadata_id handling Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 5/8] avcodec/dovi_rpu: simplify vdr type Niklas Haas
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
Move `vdr` into local scope and point only to the field we actually care
about.
---
libavcodec/dovi_rpudec.c | 35 ++++++++++++++++-------------------
1 file changed, 16 insertions(+), 19 deletions(-)
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 3e15453705..753e402dc6 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -296,7 +296,6 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
{
AVDOVIRpuDataHeader *hdr = &s->header;
GetBitContext *gb = &(GetBitContext){0};
- DOVIVdr *vdr;
int ret;
uint8_t rpu_type;
@@ -454,9 +453,9 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
prev_vdr_rpu_id);
goto fail;
}
- vdr = s->vdr[prev_vdr_rpu_id];
- s->mapping = &vdr->mapping;
+ s->mapping = &s->vdr[prev_vdr_rpu_id]->mapping;
} else {
+ AVDOVIDataMapping *mapping;
int vdr_rpu_id = get_ue_golomb_31(gb);
VALIDATE(vdr_rpu_id, 0, DOVI_MAX_DM_ID);
if (!s->vdr[vdr_rpu_id]) {
@@ -465,15 +464,13 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
return AVERROR(ENOMEM);
}
- vdr = s->vdr[vdr_rpu_id];
- s->mapping = &vdr->mapping;
-
- vdr->mapping.vdr_rpu_id = vdr_rpu_id;
- vdr->mapping.mapping_color_space = get_ue_golomb_31(gb);
- vdr->mapping.mapping_chroma_format_idc = get_ue_golomb_31(gb);
+ s->mapping = mapping = &s->vdr[vdr_rpu_id]->mapping;
+ mapping->vdr_rpu_id = vdr_rpu_id;
+ mapping->mapping_color_space = get_ue_golomb_31(gb);
+ mapping->mapping_chroma_format_idc = get_ue_golomb_31(gb);
for (int c = 0; c < 3; c++) {
- AVDOVIReshapingCurve *curve = &vdr->mapping.curves[c];
+ AVDOVIReshapingCurve *curve = &mapping->curves[c];
int num_pivots_minus_2 = get_ue_golomb_31(gb);
int pivot = 0;
@@ -487,28 +484,28 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
if (use_nlq) {
int nlq_pivot = 0;
- vdr->mapping.nlq_method_idc = get_bits(gb, 3);
+ mapping->nlq_method_idc = get_bits(gb, 3);
for (int i = 0; i < 2; i++) {
nlq_pivot += get_bits(gb, hdr->bl_bit_depth);
- vdr->mapping.nlq_pivots[i] = av_clip_uint16(nlq_pivot);
+ mapping->nlq_pivots[i] = av_clip_uint16(nlq_pivot);
}
/**
* The patent mentions another legal value, NLQ_MU_LAW, but it's
* not documented anywhere how to parse or apply that type of NLQ.
*/
- VALIDATE(vdr->mapping.nlq_method_idc, 0, AV_DOVI_NLQ_LINEAR_DZ);
+ VALIDATE(mapping->nlq_method_idc, 0, AV_DOVI_NLQ_LINEAR_DZ);
} else {
- vdr->mapping.nlq_method_idc = AV_DOVI_NLQ_NONE;
+ mapping->nlq_method_idc = AV_DOVI_NLQ_NONE;
}
- vdr->mapping.num_x_partitions = get_ue_golomb_long(gb) + 1;
- vdr->mapping.num_y_partitions = get_ue_golomb_long(gb) + 1;
+ mapping->num_x_partitions = get_ue_golomb_long(gb) + 1;
+ mapping->num_y_partitions = get_ue_golomb_long(gb) + 1;
/* End of rpu_data_header(), start of vdr_rpu_data_payload() */
for (int c = 0; c < 3; c++) {
- AVDOVIReshapingCurve *curve = &vdr->mapping.curves[c];
+ AVDOVIReshapingCurve *curve = &mapping->curves[c];
for (int i = 0; i < curve->num_pivots - 1; i++) {
int mapping_idc = get_ue_golomb_31(gb);
VALIDATE(mapping_idc, 0, 1);
@@ -549,10 +546,10 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
if (use_nlq) {
for (int c = 0; c < 3; c++) {
- AVDOVINLQParams *nlq = &vdr->mapping.nlq[c];
+ AVDOVINLQParams *nlq = &mapping->nlq[c];
nlq->nlq_offset = get_bits(gb, hdr->el_bit_depth);
nlq->vdr_in_max = get_ue_coef(gb, hdr);
- switch (vdr->mapping.nlq_method_idc) {
+ switch (mapping->nlq_method_idc) {
case AV_DOVI_NLQ_LINEAR_DZ:
nlq->linear_deadzone_slope = get_ue_coef(gb, hdr);
nlq->linear_deadzone_threshold = get_ue_coef(gb, hdr);
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 5/8] avcodec/dovi_rpu: simplify vdr type
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
` (2 preceding siblings ...)
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 4/8] avcodec/dovi_rpudec: simplify vdr handling (cosmetic) Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 6/8] avcodec/dovi_rpu: guard ext blocks by dm_metadata_present Niklas Haas
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
Storing the color metadata alongside the data mapping is no longer
needed, so we can simplify this array's type.
---
libavcodec/dovi_rpu.h | 7 +------
libavcodec/dovi_rpudec.c | 6 +++---
libavcodec/dovi_rpuenc.c | 10 +++++-----
3 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h
index da9bd67cde..058d50c64f 100644
--- a/libavcodec/dovi_rpu.h
+++ b/libavcodec/dovi_rpu.h
@@ -75,7 +75,7 @@ typedef struct DOVIContext {
* Private fields internal to dovi_rpu.c
*/
AVDOVIColorMetadata *dm; ///< RefStruct
- struct DOVIVdr *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references
+ AVDOVIDataMapping *vdr[DOVI_MAX_DM_ID+1]; ///< RefStruct references
uint8_t *rpu_buf; ///< temporary buffer
unsigned rpu_buf_sz;
@@ -127,11 +127,6 @@ int ff_dovi_configure(DOVIContext *s, AVCodecContext *avctx);
* The following section is for internal use only. *
***************************************************/
-typedef struct DOVIVdr {
- AVDOVIDataMapping mapping;
- AVDOVIColorMetadata color;
-} DOVIVdr;
-
enum {
RPU_COEFF_FIXED = 0,
RPU_COEFF_FLOAT = 1,
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 753e402dc6..51f824d126 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -453,18 +453,18 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
prev_vdr_rpu_id);
goto fail;
}
- s->mapping = &s->vdr[prev_vdr_rpu_id]->mapping;
+ s->mapping = s->vdr[prev_vdr_rpu_id];
} else {
AVDOVIDataMapping *mapping;
int vdr_rpu_id = get_ue_golomb_31(gb);
VALIDATE(vdr_rpu_id, 0, DOVI_MAX_DM_ID);
if (!s->vdr[vdr_rpu_id]) {
- s->vdr[vdr_rpu_id] = ff_refstruct_allocz(sizeof(DOVIVdr));
+ s->vdr[vdr_rpu_id] = ff_refstruct_allocz(sizeof(AVDOVIDataMapping));
if (!s->vdr[vdr_rpu_id])
return AVERROR(ENOMEM);
}
- s->mapping = mapping = &s->vdr[vdr_rpu_id]->mapping;
+ s->mapping = mapping = s->vdr[vdr_rpu_id];
mapping->vdr_rpu_id = vdr_rpu_id;
mapping->mapping_color_space = get_ue_golomb_31(gb);
mapping->mapping_chroma_format_idc = get_ue_golomb_31(gb);
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 59e8ed6e3e..8fceefe2c1 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -465,7 +465,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
vdr_rpu_id = -1;
for (int i = 0; i <= DOVI_MAX_DM_ID; i++) {
- if (s->vdr[i] && !memcmp(&s->vdr[i]->mapping, mapping, sizeof(*mapping))) {
+ 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)) {
@@ -474,7 +474,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
if (!s->vdr[vdr_rpu_id]) {
- s->vdr[vdr_rpu_id] = ff_refstruct_allocz(sizeof(DOVIVdr));
+ s->vdr[vdr_rpu_id] = ff_refstruct_allocz(sizeof(AVDOVIDataMapping));
if (!s->vdr[vdr_rpu_id])
return AVERROR(ENOMEM);
}
@@ -507,7 +507,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
vdr_dm_metadata_present = memcmp(color, &ff_dovi_color_default, sizeof(*color));
- use_prev_vdr_rpu = !memcmp(&s->vdr[vdr_rpu_id]->mapping, mapping, sizeof(*mapping));
+ use_prev_vdr_rpu = !memcmp(s->vdr[vdr_rpu_id], mapping, sizeof(*mapping));
if (vdr_dm_metadata_present && !s->dm) {
s->dm = ff_refstruct_allocz(sizeof(AVDOVIColorMetadata));
@@ -563,7 +563,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
put_bits(pb, 1, vdr_dm_metadata_present);
put_bits(pb, 1, use_prev_vdr_rpu);
set_ue_golomb(pb, vdr_rpu_id);
- s->mapping = &s->vdr[vdr_rpu_id]->mapping;
+ s->mapping = s->vdr[vdr_rpu_id];
profile = s->cfg.dv_profile ? s->cfg.dv_profile : ff_dovi_guess_profile_hevc(hdr);
@@ -629,7 +629,7 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
}
}
- memcpy(&s->vdr[vdr_rpu_id]->mapping, mapping, sizeof(*mapping));
+ memcpy(s->vdr[vdr_rpu_id], mapping, sizeof(*mapping));
}
if (vdr_dm_metadata_present) {
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 6/8] avcodec/dovi_rpu: guard ext blocks by dm_metadata_present
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
` (3 preceding siblings ...)
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 5/8] avcodec/dovi_rpu: simplify vdr type Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 7/8] avcodec/dovi_rpudec: reject reserved_zero_3bits != 0 Niklas Haas
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
In the spec, dm_metadata_present also toggles all extension blocks, so
we need to move them inside the branch.
---
libavcodec/dovi_rpudec.c | 25 +++++++++++++------------
libavcodec/dovi_rpuenc.c | 21 ++++++++++++---------
2 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 51f824d126..064d74575f 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -613,22 +613,23 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
color->source_min_pq = get_bits(gb, 12);
color->source_max_pq = get_bits(gb, 12);
color->source_diagonal = get_bits(gb, 10);
- } else {
- s->color = &ff_dovi_color_default;
- }
-
- /* Parse extension blocks */
- s->num_ext_blocks = 0;
- if ((ret = parse_ext_blocks(s, gb, 1)) < 0) {
- ff_dovi_ctx_unref(s);
- return ret;
- }
- if (get_bits_left(gb) > 48 /* padding + CRC32 + terminator */) {
- if ((ret = parse_ext_blocks(s, gb, 2)) < 0) {
+ /* Parse extension blocks */
+ s->num_ext_blocks = 0;
+ if ((ret = parse_ext_blocks(s, gb, 1)) < 0) {
ff_dovi_ctx_unref(s);
return ret;
}
+
+ if (get_bits_left(gb) > 48 /* padding + CRC32 + terminator */) {
+ if ((ret = parse_ext_blocks(s, gb, 2)) < 0) {
+ ff_dovi_ctx_unref(s);
+ return ret;
+ }
+ }
+ } else {
+ s->color = &ff_dovi_color_default;
+ s->num_ext_blocks = 0;
}
return 0;
diff --git a/libavcodec/dovi_rpuenc.c b/libavcodec/dovi_rpuenc.c
index 8fceefe2c1..f7d11c9042 100644
--- a/libavcodec/dovi_rpuenc.c
+++ b/libavcodec/dovi_rpuenc.c
@@ -508,6 +508,8 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
vdr_dm_metadata_present = memcmp(color, &ff_dovi_color_default, sizeof(*color));
use_prev_vdr_rpu = !memcmp(s->vdr[vdr_rpu_id], mapping, sizeof(*mapping));
+ if (num_ext_blocks_v1 || num_ext_blocks_v2)
+ vdr_dm_metadata_present = 1;
if (vdr_dm_metadata_present && !s->dm) {
s->dm = ff_refstruct_allocz(sizeof(AVDOVIColorMetadata));
@@ -657,18 +659,19 @@ int ff_dovi_rpu_generate(DOVIContext *s, const AVDOVIMetadata *metadata,
memcpy(s->dm, color, sizeof(*color));
s->color = s->dm;
- }
-
- set_ue_golomb(pb, num_ext_blocks_v1);
- align_put_bits(pb);
- for (int i = 0; i < metadata->num_ext_blocks; i++)
- generate_ext_v1(pb, av_dovi_get_ext(metadata, i));
- if (num_ext_blocks_v2) {
- set_ue_golomb(pb, num_ext_blocks_v2);
+ /* Extension blocks */
+ set_ue_golomb(pb, num_ext_blocks_v1);
align_put_bits(pb);
for (int i = 0; i < metadata->num_ext_blocks; i++)
- generate_ext_v2(pb, av_dovi_get_ext(metadata, i));
+ generate_ext_v1(pb, av_dovi_get_ext(metadata, i));
+
+ if (num_ext_blocks_v2) {
+ set_ue_golomb(pb, num_ext_blocks_v2);
+ align_put_bits(pb);
+ for (int i = 0; i < metadata->num_ext_blocks; i++)
+ generate_ext_v2(pb, av_dovi_get_ext(metadata, i));
+ }
}
flush_put_bits(pb);
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 7/8] avcodec/dovi_rpudec: reject reserved_zero_3bits != 0
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
` (4 preceding siblings ...)
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 6/8] avcodec/dovi_rpu: guard ext blocks by dm_metadata_present Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 8/8] avcodec/dovi_rpudec: handle errors consistently Niklas Haas
2024-06-14 11:41 ` [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
This is used by future versions of the spec to implement metadata
compression. Given that we don't yet implement that spec, validate that
this is equal to 0 for now.
---
libavcodec/dovi_rpudec.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index 064d74575f..e947870d45 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -412,6 +412,7 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
int bl_bit_depth_minus8 = get_ue_golomb_31(gb);
int el_bit_depth_minus8 = get_ue_golomb_31(gb);
int vdr_bit_depth_minus8 = get_ue_golomb_31(gb);
+ int reserved_zero_3bits;
VALIDATE(bl_bit_depth_minus8, 0, 8);
VALIDATE(el_bit_depth_minus8, 0, 8);
VALIDATE(vdr_bit_depth_minus8, 0, 8);
@@ -419,7 +420,8 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
hdr->el_bit_depth = el_bit_depth_minus8 + 8;
hdr->vdr_bit_depth = vdr_bit_depth_minus8 + 8;
hdr->spatial_resampling_filter_flag = get_bits1(gb);
- skip_bits(gb, 3); /* reserved_zero_3bits */
+ reserved_zero_3bits = get_bits(gb, 3);
+ VALIDATE(reserved_zero_3bits, 0, 0);
hdr->el_spatial_resampling_filter_flag = get_bits1(gb);
hdr->disable_residual_flag = get_bits1(gb);
}
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* [FFmpeg-devel] [PATCH 8/8] avcodec/dovi_rpudec: handle errors consistently
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
` (5 preceding siblings ...)
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 7/8] avcodec/dovi_rpudec: reject reserved_zero_3bits != 0 Niklas Haas
@ 2024-06-09 15:05 ` Niklas Haas
2024-06-14 11:41 ` [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-09 15:05 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
Only flush state when we started parsing data, otherwise just error out.
Remove the 'fail' label to make this a bit less confusing to read.
---
libavcodec/dovi_rpudec.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
index e947870d45..72e4add618 100644
--- a/libavcodec/dovi_rpudec.c
+++ b/libavcodec/dovi_rpudec.c
@@ -123,7 +123,8 @@ static inline unsigned get_variable_bits(GetBitContext *gb, int n)
if (VAR < MIN || VAR > MAX) { \
av_log(s->logctx, AV_LOG_ERROR, "RPU validation failed: " \
#MIN" <= "#VAR" = %d <= "#MAX"\n", (int) VAR); \
- goto fail; \
+ ff_dovi_ctx_unref(s); \
+ return AVERROR_INVALIDDATA; \
} \
} while (0)
@@ -306,7 +307,7 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
uint8_t profile;
if (rpu_size < 5)
- goto fail;
+ return AVERROR_INVALIDDATA;
/* Container */
if (s->cfg.dv_profile == 10 /* dav1.10 */) {
@@ -360,7 +361,7 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
}
if (!rpu_size || rpu[rpu_size - 1] != 0x80)
- goto fail;
+ return AVERROR_INVALIDDATA;
if (err_recognition & AV_EF_CRCCHECK) {
uint32_t crc = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE),
@@ -368,7 +369,7 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
if (crc) {
av_log(s->logctx, AV_LOG_ERROR, "RPU CRC mismatch: %X\n", crc);
if (err_recognition & AV_EF_EXPLODE)
- goto fail;
+ return AVERROR_INVALIDDATA;
}
}
@@ -439,7 +440,8 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
profile = s->cfg.dv_profile ? s->cfg.dv_profile : ff_dovi_guess_profile_hevc(hdr);
if (profile == 5 && use_nlq) {
av_log(s->logctx, AV_LOG_ERROR, "Profile 5 RPUs should not use NLQ\n");
- goto fail;
+ ff_dovi_ctx_unref(s);
+ return AVERROR_INVALIDDATA;
}
if (use_prev_vdr_rpu) {
@@ -453,7 +455,8 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
* out as this corner case is not hit in practice */
av_log(s->logctx, AV_LOG_ERROR, "Unknown previous RPU ID: %u\n",
prev_vdr_rpu_id);
- goto fail;
+ ff_dovi_ctx_unref(s);
+ return AVERROR_INVALIDDATA;
}
s->mapping = s->vdr[prev_vdr_rpu_id];
} else {
@@ -462,8 +465,10 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
VALIDATE(vdr_rpu_id, 0, DOVI_MAX_DM_ID);
if (!s->vdr[vdr_rpu_id]) {
s->vdr[vdr_rpu_id] = ff_refstruct_allocz(sizeof(AVDOVIDataMapping));
- if (!s->vdr[vdr_rpu_id])
+ if (!s->vdr[vdr_rpu_id]) {
+ ff_dovi_ctx_unref(s);
return AVERROR(ENOMEM);
+ }
}
s->mapping = mapping = s->vdr[vdr_rpu_id];
@@ -635,8 +640,4 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
}
return 0;
-
-fail:
- ff_dovi_ctx_unref(s); /* don't leak potentially invalid state */
- return AVERROR_INVALIDDATA;
}
--
2.45.1
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures
2024-06-09 15:05 [FFmpeg-devel] [PATCH 1/8] avdovi/dovi_rpudec: handle prev_vdr_rpu_id failures Niklas Haas
` (6 preceding siblings ...)
2024-06-09 15:05 ` [FFmpeg-devel] [PATCH 8/8] avcodec/dovi_rpudec: handle errors consistently Niklas Haas
@ 2024-06-14 11:41 ` Niklas Haas
7 siblings, 0 replies; 9+ messages in thread
From: Niklas Haas @ 2024-06-14 11:41 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
On Sun, 09 Jun 2024 17:05:46 +0200 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> From: Niklas Haas <git@haasn.dev>
>
> According to the spec, missing previous VDR RPU IDs do not constitute an
> error, but we should instead fallback first to VDR RPU with ID 0, and
> failing that, synthesize "neutral" metadata.
>
> That's nontrivial though as the resulting metadata will be dependent on
> other properties of the RPU, and this case is not hit in practice so
> I'll defer it to a rainy day.
> ---
> libavcodec/dovi_rpudec.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/libavcodec/dovi_rpudec.c b/libavcodec/dovi_rpudec.c
> index 7c7eda9d09..d1dcc3a262 100644
> --- a/libavcodec/dovi_rpudec.c
> +++ b/libavcodec/dovi_rpudec.c
> @@ -444,7 +444,12 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size,
> if (use_prev_vdr_rpu) {
> int prev_vdr_rpu_id = get_ue_golomb_31(gb);
> VALIDATE(prev_vdr_rpu_id, 0, DOVI_MAX_DM_ID);
> + if (!s->vdr[prev_vdr_rpu_id])
> + prev_vdr_rpu_id = 0;
> if (!s->vdr[prev_vdr_rpu_id]) {
> + /* FIXME: Technically, the spec says that in this case we should
> + * synthesize "neutral" vdr metadata, but easier to just error
> + * out as this corner case is not hit in practice */
> av_log(s->logctx, AV_LOG_ERROR, "Unknown previous RPU ID: %u\n",
> prev_vdr_rpu_id);
> goto fail;
> --
> 2.45.1
>
Ping for review, otherwis will merge soon as it's a lot of relatively
low-hanging fruit that fixes current deviations with the spec.
_______________________________________________
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".
^ permalink raw reply [flat|nested] 9+ messages in thread