diff mbox

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

Message ID 20191229233819.17510-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 29, 2019, 11:38 p.m. UTC
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@niedermayer.cc>
---
 libavcodec/avcodec.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

James Almer Dec. 30, 2019, 12:19 a.m. UTC | #1
On 12/29/2019 8:38 PM, Michael Niedermayer wrote:
> 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@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

Why not zero? 0xFFFFFFFF could very well be a valid tag (even if
unlikely), and we always terminate arrays with 0.

Also, should if anything be defined in internal.h, and not in a public
header.

>  } AVCodec;
>  
>  #if FF_API_CODEC_GET_SET
>
Carl Eugen Hoyos Dec. 30, 2019, 12:32 a.m. UTC | #2
Am Mo., 30. Dez. 2019 um 01:19 Uhr schrieb James Almer <jamrial@gmail.com>:
>
> On 12/29/2019 8:38 PM, Michael Niedermayer wrote:
> > 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@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
>
> Why not zero? 0xFFFFFFFF could very well be a valid tag (even if
> unlikely), and we always terminate arrays with 0.

0 is a valid codec tag in avi.

Carl Eugen
James Almer Dec. 30, 2019, 12:49 a.m. UTC | #3
On 12/29/2019 9:32 PM, Carl Eugen Hoyos wrote:
> Am Mo., 30. Dez. 2019 um 01:19 Uhr schrieb James Almer <jamrial@gmail.com>:
>>
>> On 12/29/2019 8:38 PM, Michael Niedermayer wrote:
>>> 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@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
>>
>> Why not zero? 0xFFFFFFFF could very well be a valid tag (even if
>> unlikely), and we always terminate arrays with 0.
> 
> 0 is a valid codec tag in avi.
> 
> Carl Eugen

Alright, then -1 is fine. It can always be changed to something else if
needed.
Nicolas George Dec. 30, 2019, 9:38 a.m. UTC | #4
Michael Niedermayer (12019-12-30):
> 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@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

AV_ prefix please.

>  } AVCodec;
>  
>  #if FF_API_CODEC_GET_SET

Regards,
Michael Niedermayer Jan. 21, 2020, 6:43 p.m. UTC | #5
On Mon, Dec 30, 2019 at 10:38:11AM +0100, Nicolas George wrote:
> Michael Niedermayer (12019-12-30):
> > 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@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
> 
> AV_ prefix please.

will apply with that change

thanks

[...]
Anton Khirnov Jan. 21, 2020, 6:48 p.m. UTC | #6
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@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.
Michael Niedermayer Jan. 21, 2020, 7:30 p.m. UTC | #7
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@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.
In this initial patch codec_tags was not really intended to be public
and i can certainly move CODEC_TAGS_END to a internal header if preferred.
but this is a change that we will likely undo in the future ...

Thanks

[...]
James Almer Jan. 21, 2020, 7:39 p.m. UTC | #8
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@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.

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.

> In this initial patch codec_tags was not really intended to be public
> and i can certainly move CODEC_TAGS_END to a internal header if preferred.
> but this is a change that we will likely undo in the future ...
> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer Jan. 21, 2020, 8:39 p.m. UTC | #9
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@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?

Thanks


[...]
James Almer Jan. 21, 2020, 8:40 p.m. UTC | #10
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Anton Khirnov Jan. 22, 2020, 4:51 p.m. UTC | #11
Quoting Michael Niedermayer (2020-01-21 21:39:03)
> 
> 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?

No objections from me.
Michael Niedermayer Jan. 22, 2020, 7:22 p.m. UTC | #12
On Tue, Jan 21, 2020 at 09:39:03PM +0100, 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@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?

seems everyone is ok with it, so ill go ahead and apply it with these changes


[...]
diff mbox

Patch

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
 } AVCodec;
 
 #if FF_API_CODEC_GET_SET