Re: [FFmpeg-devel] [PATCH 3/7] proresdec2: use VLC for level instead of EC switch

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Christophe Gisquet:
> Hello,
> 
> Le ven. 8 sept. 2023 à 11:57, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> a écrit :
>>>> +#define CACHED_BITSTREAM_READER 1
>>>
>>> This should be in the commit switching to the cached bitstream reader.
>>
>> Correction: This header is included in videotoolbox.c and there is other
>> stuff that also includes get_bits.h included in said file (and currently
>> gets included before proresdec.h). This means that proresdec2.c and
>> videotoolbox.c will have different opinions on what a GetBitContext is:
>> It will be the non-cached one in videotoolbox.c and the cached one in
>> proresdec2.c. This will work in practice, because ProresContext does not
>> need the complete GetBitContext type at all (it does not contain a
>> GetBitContext at all), so offsets are not affected. But it is
>> nevertheless undefined behaviour and could become dangerous when using LTO.
>>
>> So you should switch the type of the pointer to BitstreamContextBE* in
>> proresdec2.h. Furthermore, you can either include bitstream.h in
>> proresdec.h or (IMO better) use a forward declaration and struct
>> BitstreamContextBE* in the function pointer without including get_bits.h
>> in the header at all.
> 
> On that point only: I don't recall (yes that's 3+ years old) the issue
> being videotoolbox, it didn't have that include back when I wrote this
> code.
> 
> It's a very faint recollection, and I don't find proof in the ffmpeg
> code today or of 3+ years ago, but the problem you mention was
> happening with the encoder instead. So maybe the fix now is needed
> only by videotoolbox then.
> 

The videotoolbox prores hwaccel has only been added in November 2021;
before that, nobody but proresdec2.c included proresdec.h, so that there
was no issue with GetBitContext being cached or not in different files.

Another solution would be to use void* instead of GetBitContext* in the
header and in the implementation and then convert this void* to
GetBitContext* in the function.

I do not know what you mean by "the encoder instead". What problem
happens with the encoder? Why would the encoder include proresdec.h at
all and why would it be affected by changes to the decoder?

- Andreas

_______________________________________________
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".