[FFmpeg-devel] [PATCH v4 4/5] lavc: Implement Dolby Vision RPU parsing

[Niklas Haas]

On Sat, 11 Dec 2021 15:02:14 +0100 Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
> verification

Fixed

> lavu

Fixed

> Why is this always compiled when only the HEVC decoder needs it?
> (The way it is now will cause linking errors when CONFIG_GOLOMB is false.)

In principle, parsing the RPU contents is orthogonal from their delivery
within HEVC. According to Dolby the same RPU may be attached to AVC
(though admittedly nobody does so), and VVC also supports it (although
this one is not implemented in lavc yet).

How about instead adding CONFIG_DOLBYVISION as an extra configure option
(similar to CONFIG_GOLOMB), and have hevc_decoder auto-select it?

> Just to be sure: sizeof(AVDOVIMetadata) is supposed to be part of the ABI?

There's actually an argument to be made here both ways. IIRC there are
sometimes unparsed bytes left at the end of the RPU. If somebody figures
out what these are, we might be able to parse them (and add them to the
struct) in the future.

There's also the question of the extension blocks - which currently
don't seem interesting for any client and largely redundant with other
forms of side data.

Maybe it makes sense to have the sizeof this struct not be part of the
ABI.

> "The evaluations of the initialization list expressions are
> indeterminately sequenced with respect to one another and thus the order
> in which any side effects occur is unspecified." (C11, 6.7.9 23 (older
> standards contained equivalent statementes))

Fixed

> avpriv_request_sample()

Fixed

> Does the context not need to be reset upon allocation error?

I think this is subjective. The reason I reset the context for
validation errors is to avoid exposing clients to potentially illegal
values (like num_pivots exceeding the bounds of the static array).

For an allocation error, it's arguably better to keep the previously
decoded (and therefore still valid) state intact? I doubt it matters in
practice because any process which fails to allocate this small amount
of memory is probably not going to get very far anyways.

> if (offset > INT_MAX)

Fixed