[FFmpeg-devel] [PATCH] codec_desc: mark CFHD as intra-only

[James Almer]

On 6/9/2020 6:06 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-06-08 18:30:16)
>> On 6/8/2020 10:46 AM, James Almer wrote:
>>> On 6/8/2020 7:54 AM, Anton Khirnov wrote:
>>>> Quoting Hendrik Leppkes (2020-06-08 12:42:08)
>>>>> On Mon, Jun 8, 2020 at 12:22 PM Anton Khirnov <anton at khirnov.net> wrote:
>>>>>>
>>>>>> This stops update_thread_context() from being called with frame
>>>>>> threading, which causes races.
>>>>>> ---
>>>>>>  libavcodec/codec_desc.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
>>>>>> index 9f8847544f..5117984c75 100644
>>>>>> --- a/libavcodec/codec_desc.c
>>>>>> +++ b/libavcodec/codec_desc.c
>>>>>> @@ -1520,7 +1520,7 @@ static const AVCodecDescriptor codec_descriptors[] = {
>>>>>>          .type      = AVMEDIA_TYPE_VIDEO,
>>>>>>          .name      = "cfhd",
>>>>>>          .long_name = NULL_IF_CONFIG_SMALL("Cineform HD"),
>>>>>> -        .props     = AV_CODEC_PROP_LOSSY,
>>>>>> +        .props     = AV_CODEC_PROP_LOSSY | AV_CODEC_PROP_INTRA_ONLY,
>>>>>>      },
>>>>>>      {
>>>>>>          .id        = AV_CODEC_ID_TRUEMOTION2RT,
>>>>>> --
>>>>>> 2.26.2
>>>>>>
>>>>>
>>>>> A codec property influencing a decoder implementation behavior seems
>>>>> iffy at best, doesn't it?
>>>>> What if I make an intra-only implementation for a codec that
>>>>> theoretically supports more? The information no longer matches.
>>>>>
>>>>> Flags changing behavior of an implementation should really be on AVCodec.
>>>>
>>>> I generally agree, but that condition is already there and changing it
>>>> to be more robust is not entirely trivial. I am planning to get to that
>>>> as a part of some other threading work, but I did not want it to delay
>>>> my other set which is blocked by this.
>>>
>>> Maybe we could partially revert 13b1bbff0b (the intra only part) and
>>> re-purpose the flag to also apply to decoders? Or only decoders,
>>> whatever is best.
>>>
>>> We still can seeing 4.3 wasn't tagged.
>>
>> This is one way it could be implemented (attaching it as a diff since as
>> patches it would need to be split in at least two).
>>
>> In short, marking all implementations of intra-only codecs as such with
>> the relevant codec cap during static init, and then manually do the same
>> for all intra only implementations of codecs that could support more.
> 
> I don't think this needs to be visible externally, since it's only
> meaningful for internal use. I'm wondering if the presence of
> update_thread_context() callback won't be sufficient for this.

True, it could be a caps_internal. But take for example the possibility
of an external library with a fully featured decoder getting a wrapper,
this being a public capability would let the user choose it over the
internal one, same as how it can choose a stable decoder over an
experimental one, or a software one over an hybrid/hw one.

The cap is there and has been for some time, so silently undoing the
deprecation and finally give it some good use is a good approach to
solve this.