[FFmpeg-devel] [PATCH v2 03/14] vvcdec: add sps, pps, sh parser

[James Almer]

On 7/7/2023 12:48 PM, Nuo Mi wrote:
> Hi James,
> thank you for the review.
> 
> On Fri, Jul 7, 2023 at 10:28 PM James Almer <jamrial at gmail.com> wrote:
> 
>> On 7/7/2023 11:05 AM, Nuo Mi wrote:
>>> ---
>>>    libavcodec/vvc/Makefile   |    4 +-
>>>    libavcodec/vvc/vvc_data.c | 3295 +++++++++++++++++++++++++++++++++++
>>>    libavcodec/vvc/vvc_data.h |   69 +
>>>    libavcodec/vvc/vvc_ps.c   | 3436 +++++++++++++++++++++++++++++++++++++
>>>    libavcodec/vvc/vvc_ps.h   |  828 +++++++++
>>>    libavcodec/vvc/vvcdec.h   |    3 +
>>>    6 files changed, 7634 insertions(+), 1 deletion(-)
>>>    create mode 100644 libavcodec/vvc/vvc_data.c
>>>    create mode 100644 libavcodec/vvc/vvc_data.h
>>>    create mode 100644 libavcodec/vvc/vvc_ps.c
>>>    create mode 100644 libavcodec/vvc/vvc_ps.h
>>
>> I thought the plan for v2 was to use CBS? Hence you waiting until it was
>> pushed.
> 
> No. The software decoder depends on frame split, I am waiting for it. :)

Why do you need that? You can use the receive_frame() API instead of 
decode(), and only request new packets after you're done with the last 
one you got.

> 
>> CBS can store derived values and state, and you can always pass it a
>> split AU in a loop if needed.
>>
> We have many problems when if we use CBS as a parameter parser
> 1. The derived value is only needed by the software decoder. like
> VVCPH.lmcs_fwd_lut and VVCPPS.column_width. Storing it in the CBS structure
> may have performance implications in on other user cases since they never
> use it. for example, Metadata rewriter
> 2. CBS strictly follows the spec name, making it hard to write a loop like
> this
> https://github.com/ffvvc/FFmpeg/blob/ae76f43e2e47426544d4dedae1cdc3e16f546458/libavcodec/vvc/vvc_ctu.c#L2136.
> We can use a derived variable, but this makes the variable duplicate the
> original one.

You can store decoder only derived values in a decoder struct, and have 
CBS store derived values required for header parsing within itself (As 
is the case with all the relevant fields at the end of H266RawPPS), plus 
any other that calculated within CBS itself would simplify things for 
users like the decoder.

> 3. There are so many intermediate names in cbs like abc_minus1. If we use
> it directly, we may have need to calculate xxx_minus1 + 1 every time. If we
> save abc as a derived value. It will be duplicated again.

Same thing here. After getting a parsed fragment from CBS, you can 
derive all the minus/plus values once and store them in decoder only 
structs, to be used afterwards instead of those in the raw CBS structs.

> 4. Using CBS will not save too much code or logic in some cases. Take the
> Scaling list as an example, you need a for loop in CBS to read the syntax.
> Then you need a duplicated loop in the software decoder to reconstruct the
> scaling list.

The core gain is to prevent the duplication of all the header parsing 
code, and have a single point of failure and source of bugs.

The h264 and hevc decoders predate CBS, so those were obviously written 
as standalone modules, but this one doesn't, so ideally we should 
prevent duplicating huge bitstream parsing functions a third time.
Look at AV1, which even if not complete (No software implementation yet) 
has plenty of logic to derive values and tables CBS doesn't need to care 
about, but the hwaccel backends (and the eventual software 
implementation once it's ported from libdav1d) obviously need.

> 
> Again, if Mark agrees and nobody rejects it, I can send out v3 to use it.
> 
> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".