| Message ID | AS1PR01MB9564A756D9A92805D2F8C0088F139@AS1PR01MB9564.eurprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/internal: Move FF_CODEC_CAP_* to a new header codec_internal.h | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
On Fri, Mar 18, 2022 at 11:52:54AM +0100, Andreas Rheinhardt wrote: > They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. > This decreases sizeof(FFCodec), which is important given that > FFCodecs reside in .data.rel.ro in case of ELF with > position-independent code which is always loaded and can't be shared > between processes. > They are currently only used by the fuzzer, if there is no other use, iam not sure. But i agree shareable memory would be better for them [...] > diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c > index 288aa63313..77f4bb8dd8 100644 > --- a/tools/target_dec_fuzzer.c > +++ b/tools/target_dec_fuzzer.c > @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { > ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; > ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > +#if CONFIG_OSSFUZZ > ctx->codec_tag = bytestream2_get_le32(&gbc); > if (c->codec_tags) { > int n; > for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); > ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; > } > +#endif > keyframes = bytestream2_get_le64(&gbc); > request_channel_layout = bytestream2_get_le64(&gbc); > how does the fuzzer work without the fuzzer ? thx [...]
On Fri, Mar 18, 2022 at 02:13:16PM +0100, Michael Niedermayer wrote: > On Fri, Mar 18, 2022 at 11:52:54AM +0100, Andreas Rheinhardt wrote: > > They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. > > This decreases sizeof(FFCodec), which is important given that > > FFCodecs reside in .data.rel.ro in case of ELF with > > position-independent code which is always loaded and can't be shared > > between processes. > > > > They are currently only used by the fuzzer, if there is no other > use, iam not sure. But i agree shareable memory would be better for them > > > [...] > > diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c > > index 288aa63313..77f4bb8dd8 100644 > > --- a/tools/target_dec_fuzzer.c > > +++ b/tools/target_dec_fuzzer.c > > @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { > > ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > > ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; > > ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > > +#if CONFIG_OSSFUZZ > > ctx->codec_tag = bytestream2_get_le32(&gbc); > > if (c->codec_tags) { > > int n; > > for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); > > ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; > > } > > +#endif > > keyframes = bytestream2_get_le64(&gbc); > > request_channel_layout = bytestream2_get_le64(&gbc); > > > > how does the fuzzer work without the fuzzer ? or is the idea to build test this even without oss-fuzz somehow? [...]
Michael Niedermayer: > On Fri, Mar 18, 2022 at 11:52:54AM +0100, Andreas Rheinhardt wrote: >> They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. >> This decreases sizeof(FFCodec), which is important given that >> FFCodecs reside in .data.rel.ro in case of ELF with >> position-independent code which is always loaded and can't be shared >> between processes. >> > > They are currently only used by the fuzzer, if there is no other > use, iam not sure. But i agree shareable memory would be better for them > GCC by default aligns big enough objects to 32* (so while sizeof(codec) need not be a multiple of said alignment, the actual object is still padded to said alignment) and it was aligned to 32 before the addition of ch_layouts (for x64), so adding it increased the size by 32 (and resulted in over 21KiB size increase here on a really not-all-encompassing build). Optimizing codec_tags away would reverse this. > > [...] >> diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c >> index 288aa63313..77f4bb8dd8 100644 >> --- a/tools/target_dec_fuzzer.c >> +++ b/tools/target_dec_fuzzer.c >> @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { >> ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; >> ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; >> ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; >> +#if CONFIG_OSSFUZZ >> ctx->codec_tag = bytestream2_get_le32(&gbc); >> if (c->codec_tags) { >> int n; >> for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); >> ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; >> } >> +#endif >> keyframes = bytestream2_get_le64(&gbc); >> request_channel_layout = bytestream2_get_le64(&gbc); >> > > how does the fuzzer work without the fuzzer ? > You could just not use --enable-ossfuzz, but nevertheless want to "make tools/target_dec_foo_fuzzer" (with custom fuzzer CFLAGS/LDFLAGS). But it is not really the intended usecase, so I made sure it compiles, but nothing more. - Andreas *: The ELF-x64-ABI only requires 16, but it uses 32 for compatibility with ancient versions of GCC which required 32. One can use -malign-data=abi to make it only use 16.
Michael Niedermayer: > On Fri, Mar 18, 2022 at 02:13:16PM +0100, Michael Niedermayer wrote: >> On Fri, Mar 18, 2022 at 11:52:54AM +0100, Andreas Rheinhardt wrote: >>> They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. >>> This decreases sizeof(FFCodec), which is important given that >>> FFCodecs reside in .data.rel.ro in case of ELF with >>> position-independent code which is always loaded and can't be shared >>> between processes. >>> >> >> They are currently only used by the fuzzer, if there is no other >> use, iam not sure. But i agree shareable memory would be better for them >> >> >> [...] >>> diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c >>> index 288aa63313..77f4bb8dd8 100644 >>> --- a/tools/target_dec_fuzzer.c >>> +++ b/tools/target_dec_fuzzer.c >>> @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { >>> ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; >>> ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; >>> ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; >>> +#if CONFIG_OSSFUZZ >>> ctx->codec_tag = bytestream2_get_le32(&gbc); >>> if (c->codec_tags) { >>> int n; >>> for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); >>> ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; >>> } >>> +#endif >>> keyframes = bytestream2_get_le64(&gbc); >>> request_channel_layout = bytestream2_get_le64(&gbc); >>> >> >> how does the fuzzer work without the fuzzer ? > > or is the idea to build test this even without oss-fuzz somehow? > Yes, this #if CONFIG_OSSFUZZ is just intended to make it still compile in this case. - Andreas
Andreas Rheinhardt: > They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. > This decreases sizeof(FFCodec), which is important given that > FFCodecs reside in .data.rel.ro in case of ELF with > position-independent code which is always loaded and can't be shared > between processes. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/bitpacked_dec.c | 5 +---- > libavcodec/codec_internal.h | 10 ++++++++++ > libavcodec/hapdec.c | 13 +++++-------- > tools/target_dec_fuzzer.c | 2 ++ > 4 files changed, 18 insertions(+), 12 deletions(-) > > diff --git a/libavcodec/bitpacked_dec.c b/libavcodec/bitpacked_dec.c > index 419550dfe0..b62d88fa8f 100644 > --- a/libavcodec/bitpacked_dec.c > +++ b/libavcodec/bitpacked_dec.c > @@ -151,9 +151,6 @@ const FFCodec ff_bitpacked_decoder = { > .init = bitpacked_init_decoder, > .decode = bitpacked_decode, > .p.capabilities = AV_CODEC_CAP_FRAME_THREADS, > - .codec_tags = (const uint32_t []){ > - MKTAG('U', 'Y', 'V', 'Y'), > - FF_CODEC_TAGS_END, > - }, > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE, > + FF_CODEC_TAGS(MKTAG('U', 'Y', 'V', 'Y')) > }; > diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h > index 596cdbebd2..b6b5b05b44 100644 > --- a/libavcodec/codec_internal.h > +++ b/libavcodec/codec_internal.h > @@ -21,6 +21,7 @@ > > #include <stdint.h> > > +#include "config.h" > #include "libavutil/attributes.h" > #include "codec.h" > > @@ -74,10 +75,16 @@ > */ > #define FF_CODEC_CAP_SETS_FRAME_PROPS (1 << 8) > > +#if CONFIG_OSSFUZZ > /** > * FFCodec.codec_tags termination value > */ > #define FF_CODEC_TAGS_END -1 > +#define FF_CODEC_TAGS(...) \ > + .codec_tags = (const uint32_t[]){ __VA_ARGS__, FF_CODEC_TAGS_END }, > +#else > +#define FF_CODEC_TAGS(...) > +#endif > > typedef struct FFCodecDefault { > const char *key; > @@ -196,10 +203,13 @@ typedef struct FFCodec { > */ > const struct AVCodecHWConfigInternal *const *hw_configs; > > +#if CONFIG_OSSFUZZ > /** > * List of supported codec_tags, terminated by FF_CODEC_TAGS_END. > + * Should be defined with the FF_CODEC_TAGS() macro. > */ > const uint32_t *codec_tags; > +#endif > } FFCodec; > > static av_always_inline const FFCodec *ffcodec(const AVCodec *codec) > diff --git a/libavcodec/hapdec.c b/libavcodec/hapdec.c > index 4a7ac15a8e..72f922bc5b 100644 > --- a/libavcodec/hapdec.c > +++ b/libavcodec/hapdec.c > @@ -486,12 +486,9 @@ const FFCodec ff_hap_decoder = { > AV_CODEC_CAP_DR1, > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | > FF_CODEC_CAP_INIT_CLEANUP, > - .codec_tags = (const uint32_t []){ > - MKTAG('H','a','p','1'), > - MKTAG('H','a','p','5'), > - MKTAG('H','a','p','Y'), > - MKTAG('H','a','p','A'), > - MKTAG('H','a','p','M'), > - FF_CODEC_TAGS_END, > - }, > + FF_CODEC_TAGS(MKTAG('H','a','p','1'), > + MKTAG('H','a','p','5'), > + MKTAG('H','a','p','Y'), > + MKTAG('H','a','p','A'), > + MKTAG('H','a','p','M')) > }; > diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c > index 288aa63313..77f4bb8dd8 100644 > --- a/tools/target_dec_fuzzer.c > +++ b/tools/target_dec_fuzzer.c > @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { > ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; > ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; > +#if CONFIG_OSSFUZZ > ctx->codec_tag = bytestream2_get_le32(&gbc); > if (c->codec_tags) { > int n; > for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); > ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; > } > +#endif > keyframes = bytestream2_get_le64(&gbc); > request_channel_layout = bytestream2_get_le64(&gbc); > Will apply tomorrow unless there are objections. - Andreas
diff --git a/libavcodec/bitpacked_dec.c b/libavcodec/bitpacked_dec.c index 419550dfe0..b62d88fa8f 100644 --- a/libavcodec/bitpacked_dec.c +++ b/libavcodec/bitpacked_dec.c @@ -151,9 +151,6 @@ const FFCodec ff_bitpacked_decoder = { .init = bitpacked_init_decoder, .decode = bitpacked_decode, .p.capabilities = AV_CODEC_CAP_FRAME_THREADS, - .codec_tags = (const uint32_t []){ - MKTAG('U', 'Y', 'V', 'Y'), - FF_CODEC_TAGS_END, - }, .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE, + FF_CODEC_TAGS(MKTAG('U', 'Y', 'V', 'Y')) }; diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h index 596cdbebd2..b6b5b05b44 100644 --- a/libavcodec/codec_internal.h +++ b/libavcodec/codec_internal.h @@ -21,6 +21,7 @@ #include <stdint.h> +#include "config.h" #include "libavutil/attributes.h" #include "codec.h" @@ -74,10 +75,16 @@ */ #define FF_CODEC_CAP_SETS_FRAME_PROPS (1 << 8) +#if CONFIG_OSSFUZZ /** * FFCodec.codec_tags termination value */ #define FF_CODEC_TAGS_END -1 +#define FF_CODEC_TAGS(...) \ + .codec_tags = (const uint32_t[]){ __VA_ARGS__, FF_CODEC_TAGS_END }, +#else +#define FF_CODEC_TAGS(...) +#endif typedef struct FFCodecDefault { const char *key; @@ -196,10 +203,13 @@ typedef struct FFCodec { */ const struct AVCodecHWConfigInternal *const *hw_configs; +#if CONFIG_OSSFUZZ /** * List of supported codec_tags, terminated by FF_CODEC_TAGS_END. + * Should be defined with the FF_CODEC_TAGS() macro. */ const uint32_t *codec_tags; +#endif } FFCodec; static av_always_inline const FFCodec *ffcodec(const AVCodec *codec) diff --git a/libavcodec/hapdec.c b/libavcodec/hapdec.c index 4a7ac15a8e..72f922bc5b 100644 --- a/libavcodec/hapdec.c +++ b/libavcodec/hapdec.c @@ -486,12 +486,9 @@ const FFCodec ff_hap_decoder = { AV_CODEC_CAP_DR1, .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP, - .codec_tags = (const uint32_t []){ - MKTAG('H','a','p','1'), - MKTAG('H','a','p','5'), - MKTAG('H','a','p','Y'), - MKTAG('H','a','p','A'), - MKTAG('H','a','p','M'), - FF_CODEC_TAGS_END, - }, + FF_CODEC_TAGS(MKTAG('H','a','p','1'), + MKTAG('H','a','p','5'), + MKTAG('H','a','p','Y'), + MKTAG('H','a','p','A'), + MKTAG('H','a','p','M')) }; diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c index 288aa63313..77f4bb8dd8 100644 --- a/tools/target_dec_fuzzer.c +++ b/tools/target_dec_fuzzer.c @@ -279,12 +279,14 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { ctx->sample_rate = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; ctx->ch_layout.nb_channels = (unsigned)bytestream2_get_le32(&gbc) % FF_SANE_NB_CHANNELS; ctx->block_align = bytestream2_get_le32(&gbc) & 0x7FFFFFFF; +#if CONFIG_OSSFUZZ ctx->codec_tag = bytestream2_get_le32(&gbc); if (c->codec_tags) { int n; for (n = 0; c->codec_tags[n] != FF_CODEC_TAGS_END; n++); ctx->codec_tag = c->codec_tags[ctx->codec_tag % n]; } +#endif keyframes = bytestream2_get_le64(&gbc); request_channel_layout = bytestream2_get_le64(&gbc);
They are only needed for the fuzzer, so check for CONFIG_OSSFUZZ. This decreases sizeof(FFCodec), which is important given that FFCodecs reside in .data.rel.ro in case of ELF with position-independent code which is always loaded and can't be shared between processes. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/bitpacked_dec.c | 5 +---- libavcodec/codec_internal.h | 10 ++++++++++ libavcodec/hapdec.c | 13 +++++-------- tools/target_dec_fuzzer.c | 2 ++ 4 files changed, 18 insertions(+), 12 deletions(-)