[FFmpeg-devel] [PATCH 1/3] avcodec/avcodec: Add codec_tags array to AVCodec

[James Almer]

On 1/21/2020 5:39 PM, Michael Niedermayer wrote:
> On Tue, Jan 21, 2020 at 04:39:10PM -0300, James Almer wrote:
>> On 1/21/2020 4:30 PM, Michael Niedermayer wrote:
>>> On Tue, Jan 21, 2020 at 07:48:38PM +0100, Anton Khirnov wrote:
>>>> Quoting Michael Niedermayer (2019-12-30 00:38:17)
>>>>> This allows the fuzzer to target meaningfull codec tags instead
>>>>> of hunting the 4gb space, which it seems to have problems with.
>>>>>
>>>>> Suggested-by: James
>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>> ---
>>>>>  libavcodec/avcodec.h | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>> index 119b32dc1f..b0c6a8f2e3 100644
>>>>> --- a/libavcodec/avcodec.h
>>>>> +++ b/libavcodec/avcodec.h
>>>>> @@ -3634,6 +3634,12 @@ typedef struct AVCodec {
>>>>>       * The user can only access this field via avcodec_get_hw_config().
>>>>>       */
>>>>>      const struct AVCodecHWConfigInternal **hw_configs;
>>>>> +
>>>>> +    /**
>>>>> +     * List of supported codec_tags, terminated by CODEC_TAGS_END.
>>>>> +     */
>>>>> +    const uint32_t *codec_tags;
>>>>> +#define CODEC_TAGS_END -1
>>>>
>>>> Is this supposed to be public or for fuzzer use only?
>>>> If the latter, then CODEC_TAGS_END doesn't need to live in a public
>>>> header.
>>>
>>> the codec_tag field is public. So eventually the list of valid codec tags
>>> should become too.
>>
>> It's at the end of AVCodec, which is where the fields that must not be
>> used or accessed from outside of lavc reside. They are documented as
>> "can be removed at any time without warning and without bumping
>> version". So basically, it's not public.
> 
> yes, it could either be moved up or a public function to access it
> added when its decided to make it public
> 
> 
>>
>> Since you are accessing them directly from the fuzzer anyway, might as
>> well just assume the -1 termination and not bother adding a public
>> define for it. If we ever want this public then it can be added, perhaps
>> after a better design is found.
> 
> replacing a named constant by a litteral repeated -1 is making the code
> worse. (maintaince and understanding wise)
> 
> So from the smell of this thread i dont think we will agree on a public
> API and that also wasnt suggested nor is it needed ATM.
> 
> Thus i suggest we keep the codec_tags as in the original patch in the
> private part and put the CODEC_TAGS_END  with a FF prefix in 
> internal.h
> 
> That way it can be used in the fuzzer and we can in the future decide
> if this or a different API could/should become public.
> 
> is that ok with everyone?

Ok with me.

> 
> Thanks
> 
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>