[FFmpeg-devel] CBS

[Andreas Rheinhardt]

James Almer:
> On 8/6/2024 2:54 PM, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Tue, Aug 06, 2024 at 07:05:38PM +0200, Michael Niedermayer wrote:
>>>> Hi
>>>>
>>>> Did CBS win the obfuscated C contest yet?
>>>>
>>>> I was just looking at a msan issue and then looked at this:
>>>>
>>>> CHECK(FUNC_SEI(message_list)(ctx, rw, &current->message_list, 1));
>>>>
>>>>
>>>> #define CHECK(call) do { \
>>>>          err = (call); \
>>>>          if (err < 0) \
>>>>              return err; \
>>>>      } while (0)
>>>>
>>>> #define FUNC_NAME2(rw, codec, name) cbs_ ## codec ## _ ## rw ## _ ##
>>>> name
>>>> #define FUNC_NAME1(rw, codec, name) FUNC_NAME2(rw, codec, name)
>>>> #define FUNC_H264(name) FUNC_NAME1(READWRITE, h264, name)
>>>> #define FUNC_H265(name) FUNC_NAME1(READWRITE, h265, name)
>>>> #define FUNC_H266(name) FUNC_NAME1(READWRITE, h266, name)
>>>> #define FUNC_SEI(name)  FUNC_NAME1(READWRITE, sei,  name)
>>>>
>>>> #define SEI_FUNC(name, args) \
>>>> static int FUNC(name) args;  \
>>>> static int FUNC(name ## _internal)(CodedBitstreamContext *ctx, \
>>>>                                     RWContext *rw, void *cur,   \
>>>>                                     SEIMessageState *state)     \
>>>> { \
>>>>      return FUNC(name)(ctx, rw, cur, state); \
>>>> } \
>>>> static int FUNC(name) args
>>>>
>>>>
>>>> anyway, can we remove all preprocessor use from cbs ?
>>
>> I don't think that this is really obfuscated.
>>
>>>
>>> the issue iam looking at is due to
>>>
>>> SEI_FUNC(sei_pic_timing, (CodedBitstreamContext *ctx, RWContext *rw,
>>> H264RawSEIPicTiming *current, SEIMessageState *sei))
>>>
>>> having different active SPS on writing than reading, so the write code
>>> has nal_hrd_parameters_present_flag set while the read had that 0
>>> so uninitialized data is written
>>>
>>> I cannot find any match for "cbs" in MAINTAINERS, also there are no
>>> copyright
>>> with names in the cbs code.
>>
>> 1. I just sent a patch that "fixes" this.
>> 2. But actually, there is a deeper bug here: We would need to defer
>> parsing certain SEI message units to a second pass when the currently
>> active SPS is known. This can happen with spec-compliant input (and even
> 
> Is this a scenario where a slice that referenced an SPS was parsed, then
> a SEI message, then another slice that references another SPS, and the
> SEI expects the latter to be active despite it being coded before the
> slice?

Your example is not spec-compliant (at least not if the input is
supposed to only contain a single access unit). It can happen if there
is an SEI, then new parameter sets and then a slice activating these new
parameter sets. Or it can happen if there are multiple parameter sets
and an SEI followed by a slice activating a different SPS. The current
code is also wrong when parsing the first packet: Because in this case
no SPS has been activated yet, film_grain and sei_pic_timing SEI parsing
code contains a hack to make this work in the common case of a single SPS.

> 
>> more so with spec-incompliant input, which is presumably what the fuzzer
>> produced).
>>