| Message ID | 20191229233819.17510-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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
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.
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,
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 [...]
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.
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 [...]
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". >
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 [...]
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". >
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.
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 --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
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(+)