| Message ID | 20210903183913.15255-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/utils: ARGO writes 4x4 blocks without regard to the image dimensions | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 9/3/2021 3:39 PM, Michael Niedermayer wrote: > Fixes: Timeout (56sec -> 15sec) > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > 1 file changed, 32 insertions(+), 24 deletions(-) > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > index e5105ecc58..e91c554781 100644 > --- a/libavcodec/mpegutils.c > +++ b/libavcodec/mpegutils.c > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > av_freep(&mvs); > } > - Stray change. > /* TODO: export all the following to make them accessible for users (and filters) */ > if (avctx->hwaccel || !mbtype_table) > return; > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > int x,y; > +#define MB_STRING_SIZE 6 > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); > + if (!mbstring) > + return; > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > av_get_picture_type_char(pict->pict_type)); > for (y = 0; y < mb_height; y++) { > + char *mbs = mbstring; > for (x = 0; x < mb_width; x++) { > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > if (avctx->debug & FF_DEBUG_SKIP) { > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > if (count > 9) > count = 9; > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > + *mbs++ = '0' + count; This is ugly and not very obvious at first glance. Can you use lavu's bprint or something like that instead? > } > if (avctx->debug & FF_DEBUG_QP) { > - av_log(avctx, AV_LOG_DEBUG, "%2d", > - qscale_table[x + y * mb_stride]); > + int q = qscale_table[x + y * mb_stride]; > + *mbs++ = '0' + q/10; > + *mbs++ = '0' + q%10; > } > if (avctx->debug & FF_DEBUG_MB_TYPE) { > int mb_type = mbtype_table[x + y * mb_stride]; > // Type & MV direction > if (IS_PCM(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "P"); > + *mbs++ = 'P'; > else if (IS_INTRA(mb_type) && IS_ACPRED(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "A"); > + *mbs++ = 'A'; > else if (IS_INTRA4x4(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "i"); > + *mbs++ = 'i'; > else if (IS_INTRA16x16(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "I"); > + *mbs++ = 'I'; > else if (IS_DIRECT(mb_type) && IS_SKIP(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "d"); > + *mbs++ = 'd'; > else if (IS_DIRECT(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "D"); > + *mbs++ = 'D'; > else if (IS_GMC(mb_type) && IS_SKIP(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "g"); > + *mbs++ = 'g'; > else if (IS_GMC(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "G"); > + *mbs++ = 'G'; > else if (IS_SKIP(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "S"); > + *mbs++ = 'S'; > else if (!USES_LIST(mb_type, 1)) > - av_log(avctx, AV_LOG_DEBUG, ">"); > + *mbs++ = '>'; > else if (!USES_LIST(mb_type, 0)) > - av_log(avctx, AV_LOG_DEBUG, "<"); > + *mbs++ = '<'; > else { > av_assert2(USES_LIST(mb_type, 0) && USES_LIST(mb_type, 1)); > - av_log(avctx, AV_LOG_DEBUG, "X"); > + *mbs++ = 'X'; > } > > // segmentation > if (IS_8X8(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "+"); > + *mbs++ = '+'; > else if (IS_16X8(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "-"); > + *mbs++ = '-'; > else if (IS_8X16(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "|"); > + *mbs++ = '|'; > else if (IS_INTRA(mb_type) || IS_16X16(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, " "); > + *mbs++ = ' '; > else > - av_log(avctx, AV_LOG_DEBUG, "?"); > + *mbs++ = '?'; > > > if (IS_INTERLACED(mb_type)) > - av_log(avctx, AV_LOG_DEBUG, "="); > + *mbs++ = '='; > else > - av_log(avctx, AV_LOG_DEBUG, " "); > + *mbs++ = ' '; > } > } > - av_log(avctx, AV_LOG_DEBUG, "\n"); > + *mbs++ = 0; > + av_log(avctx, AV_LOG_DEBUG, "%s\n", mbstring); > } > + av_freep(&mbstring); > } > } >
Michael Niedermayer: > Fixes: Timeout (56sec -> 15sec) > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > 1 file changed, 32 insertions(+), 24 deletions(-) > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > index e5105ecc58..e91c554781 100644 > --- a/libavcodec/mpegutils.c > +++ b/libavcodec/mpegutils.c > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > av_freep(&mvs); > } > - > /* TODO: export all the following to make them accessible for users (and filters) */ > if (avctx->hwaccel || !mbtype_table) > return; > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > int x,y; > +#define MB_STRING_SIZE 6 > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); Is it guaranteed that mb_width can't be huge? (I wouldn't be surprised if there were a compile-time bound for it; this could be used for a stack allocation.) > + if (!mbstring) > + return; > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > av_get_picture_type_char(pict->pict_type)); > for (y = 0; y < mb_height; y++) { > + char *mbs = mbstring; > for (x = 0; x < mb_width; x++) { > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > if (avctx->debug & FF_DEBUG_SKIP) { > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > if (count > 9) > count = 9; > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > + *mbs++ = '0' + count; > } > if (avctx->debug & FF_DEBUG_QP) { > - av_log(avctx, AV_LOG_DEBUG, "%2d", > - qscale_table[x + y * mb_stride]); > + int q = qscale_table[x + y * mb_stride]; > + *mbs++ = '0' + q/10; > + *mbs++ = '0' + q%10; This is only equivalent to the old code if the value is in the range 0-99. Is this guaranteed? - Andreas
On Fri, Sep 03, 2021 at 09:00:22PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: Timeout (56sec -> 15sec) > > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > > 1 file changed, 32 insertions(+), 24 deletions(-) > > > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > > index e5105ecc58..e91c554781 100644 > > --- a/libavcodec/mpegutils.c > > +++ b/libavcodec/mpegutils.c > > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > av_freep(&mvs); > > } > > - > > /* TODO: export all the following to make them accessible for users (and filters) */ > > if (avctx->hwaccel || !mbtype_table) > > return; > > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > > int x,y; > > +#define MB_STRING_SIZE 6 > > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); > > Is it guaranteed that mb_width can't be huge? (I wouldn't be surprised > if there were a compile-time bound for it; this could be used for a > stack allocation.) i had no major problem creating a file with a mb_width above 40k, the failure was from allocating the image if the size was increased not anything inherent to mb_width not every codec and container allows this but some do, i picked msmpeg4v3 in nut but that is not a unique choice at all > > > + if (!mbstring) > > + return; > > > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > > av_get_picture_type_char(pict->pict_type)); > > for (y = 0; y < mb_height; y++) { > > + char *mbs = mbstring; > > for (x = 0; x < mb_width; x++) { > > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > > if (avctx->debug & FF_DEBUG_SKIP) { > > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > > if (count > 9) > > count = 9; > > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > > + *mbs++ = '0' + count; > > } > > if (avctx->debug & FF_DEBUG_QP) { > > - av_log(avctx, AV_LOG_DEBUG, "%2d", > > - qscale_table[x + y * mb_stride]); > > + int q = qscale_table[x + y * mb_stride]; > > + *mbs++ = '0' + q/10; > > + *mbs++ = '0' + q%10; > > This is only equivalent to the old code if the value is in the range > 0-99. Is this guaranteed? the code prior to this assumed so, if it exceeded it, the output would become difficult to read. So if this assumtation is wrong we would need to already fix that, Do you know of a case where this is wrong ? thx [...]
On Fri, Sep 03, 2021 at 03:45:55PM -0300, James Almer wrote: > On 9/3/2021 3:39 PM, Michael Niedermayer wrote: > > Fixes: Timeout (56sec -> 15sec) > > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > > 1 file changed, 32 insertions(+), 24 deletions(-) > > > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > > index e5105ecc58..e91c554781 100644 > > --- a/libavcodec/mpegutils.c > > +++ b/libavcodec/mpegutils.c > > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > av_freep(&mvs); > > } > > - > > Stray change. > > > /* TODO: export all the following to make them accessible for users (and filters) */ > > if (avctx->hwaccel || !mbtype_table) > > return; > > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > > int x,y; > > +#define MB_STRING_SIZE 6 > > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); > > + if (!mbstring) > > + return; > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > > av_get_picture_type_char(pict->pict_type)); > > for (y = 0; y < mb_height; y++) { > > + char *mbs = mbstring; > > for (x = 0; x < mb_width; x++) { > > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > > if (avctx->debug & FF_DEBUG_SKIP) { > > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > > if (count > 9) > > count = 9; > > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > > + *mbs++ = '0' + count; > > This is ugly and not very obvious at first glance. Can you use lavu's bprint > or something like that instead? the whole point of the patch is to eliminate the string formating overhead. (which surprisingly is significant) so as long as we have a "%d" style format in there, it at least feels like we arent doing a good job at eliminating it would moving this in a seperate function resolve this concern too ? thx [...]
Michael Niedermayer: > On Fri, Sep 03, 2021 at 09:00:22PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >>> Fixes: Timeout (56sec -> 15sec) >>> Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ >>> 1 file changed, 32 insertions(+), 24 deletions(-) >>> >>> diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c >>> index e5105ecc58..e91c554781 100644 >>> --- a/libavcodec/mpegutils.c >>> +++ b/libavcodec/mpegutils.c >>> @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ >>> >>> av_freep(&mvs); >>> } >>> - >>> /* TODO: export all the following to make them accessible for users (and filters) */ >>> if (avctx->hwaccel || !mbtype_table) >>> return; >>> @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ >>> >>> if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { >>> int x,y; >>> +#define MB_STRING_SIZE 6 >>> + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); >> >> Is it guaranteed that mb_width can't be huge? (I wouldn't be surprised >> if there were a compile-time bound for it; this could be used for a >> stack allocation.) > > i had no major problem creating a file with a mb_width above 40k, the failure > was from allocating the image if the size was increased not anything inherent > to mb_width > not every codec and container allows this but some do, i picked > msmpeg4v3 in nut but that is not a unique choice at all > Forget what I said above in parentheses: I thought that the size of a single macroblock is bounded for all codecs using this function. But mb_width is apparently the width of the picture in macroblocks. > >> >>> + if (!mbstring) >>> + return; >>> >>> av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", >>> av_get_picture_type_char(pict->pict_type)); >>> for (y = 0; y < mb_height; y++) { >>> + char *mbs = mbstring; >>> for (x = 0; x < mb_width; x++) { >>> + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); >>> if (avctx->debug & FF_DEBUG_SKIP) { >>> int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; >>> if (count > 9) >>> count = 9; >>> - av_log(avctx, AV_LOG_DEBUG, "%1d", count); >>> + *mbs++ = '0' + count; >>> } >>> if (avctx->debug & FF_DEBUG_QP) { >>> - av_log(avctx, AV_LOG_DEBUG, "%2d", >>> - qscale_table[x + y * mb_stride]); >>> + int q = qscale_table[x + y * mb_stride]; >>> + *mbs++ = '0' + q/10; >>> + *mbs++ = '0' + q%10; >> >> This is only equivalent to the old code if the value is in the range >> 0-99. Is this guaranteed? > > the code prior to this assumed so, if it exceeded it, the output would > become difficult to read. So if this assumtation is wrong we would need to > already fix that, Do you know of a case where this is wrong ? > No. - Andreas
On 9/4/2021 12:28 PM, Michael Niedermayer wrote: > On Fri, Sep 03, 2021 at 03:45:55PM -0300, James Almer wrote: >> On 9/3/2021 3:39 PM, Michael Niedermayer wrote: >>> Fixes: Timeout (56sec -> 15sec) >>> Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ >>> 1 file changed, 32 insertions(+), 24 deletions(-) >>> >>> diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c >>> index e5105ecc58..e91c554781 100644 >>> --- a/libavcodec/mpegutils.c >>> +++ b/libavcodec/mpegutils.c >>> @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ >>> av_freep(&mvs); >>> } >>> - >> >> Stray change. >> >>> /* TODO: export all the following to make them accessible for users (and filters) */ >>> if (avctx->hwaccel || !mbtype_table) >>> return; >>> @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ >>> if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { >>> int x,y; >>> +#define MB_STRING_SIZE 6 >>> + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); >>> + if (!mbstring) >>> + return; >>> av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", >>> av_get_picture_type_char(pict->pict_type)); >>> for (y = 0; y < mb_height; y++) { >>> + char *mbs = mbstring; >>> for (x = 0; x < mb_width; x++) { >>> + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); >>> if (avctx->debug & FF_DEBUG_SKIP) { >>> int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; >>> if (count > 9) >>> count = 9; >>> - av_log(avctx, AV_LOG_DEBUG, "%1d", count); >>> + *mbs++ = '0' + count; >> >> This is ugly and not very obvious at first glance. Can you use lavu's bprint >> or something like that instead? > > the whole point of the patch is to eliminate the string formating overhead. > (which surprisingly is significant) > so as long as we have a "%d" style format in there, it at least feels like we > arent doing a good job at eliminating it Is the string formatting the problem, or the constant calls to av_log()? av_bprintf() would do the string formatting, but append each character into a buffer you can then use to call av_log() once at the end. Something like av_bprint_init(&bp, MB_STRING_SIZE * mb_width + 1, UINT_MAX); [...] av_bprintf(&bp, "%1d", count); av_bprint_chars(&bp, 'P', 1); [...] av_bprint_finalize(&bp, &buf); av_log(avctx, AV_LOG_DEBUG, "%s\n" buf); av_free(buf); > > would moving this in a seperate function resolve this concern too ? If the above doesn't perform well (Or you don't feel like trying it), then an inline function or macro should be fine. > > thx > > [...] > > > _______________________________________________ > 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 Sat, Sep 04, 2021 at 01:37:26PM -0300, James Almer wrote: > On 9/4/2021 12:28 PM, Michael Niedermayer wrote: > > On Fri, Sep 03, 2021 at 03:45:55PM -0300, James Almer wrote: > > > On 9/3/2021 3:39 PM, Michael Niedermayer wrote: > > > > Fixes: Timeout (56sec -> 15sec) > > > > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > > > > 1 file changed, 32 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > > > > index e5105ecc58..e91c554781 100644 > > > > --- a/libavcodec/mpegutils.c > > > > +++ b/libavcodec/mpegutils.c > > > > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > av_freep(&mvs); > > > > } > > > > - > > > > > > Stray change. > > > > > > > /* TODO: export all the following to make them accessible for users (and filters) */ > > > > if (avctx->hwaccel || !mbtype_table) > > > > return; > > > > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > > > > int x,y; > > > > +#define MB_STRING_SIZE 6 > > > > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); > > > > + if (!mbstring) > > > > + return; > > > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > > > > av_get_picture_type_char(pict->pict_type)); > > > > for (y = 0; y < mb_height; y++) { > > > > + char *mbs = mbstring; > > > > for (x = 0; x < mb_width; x++) { > > > > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > > > > if (avctx->debug & FF_DEBUG_SKIP) { > > > > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > > > > if (count > 9) > > > > count = 9; > > > > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > > > > + *mbs++ = '0' + count; > > > > > > This is ugly and not very obvious at first glance. Can you use lavu's bprint > > > or something like that instead? > > > > the whole point of the patch is to eliminate the string formating overhead. > > (which surprisingly is significant) > > so as long as we have a "%d" style format in there, it at least feels like we > > arent doing a good job at eliminating it > > Is the string formatting the problem, or the constant calls to av_log()? > > av_bprintf() would do the string formatting, but append each character into > a buffer you can then use to call av_log() once at the end. > Something like > > av_bprint_init(&bp, MB_STRING_SIZE * mb_width + 1, UINT_MAX); > [...] > av_bprintf(&bp, "%1d", count); > av_bprint_chars(&bp, 'P', 1); > [...] > av_bprint_finalize(&bp, &buf); > av_log(avctx, AV_LOG_DEBUG, "%s\n" buf); > av_free(buf); > > > > > would moving this in a seperate function resolve this concern too ? > > If the above doesn't perform well (Or you don't feel like trying it), then > an inline function or macro should be fine. ATM a bunch of issues accumulated that i want to look into so i would favor keeping this simple. But if you want to look into this, you surely can thx [...]
On Sat, Sep 04, 2021 at 01:37:26PM -0300, James Almer wrote: > On 9/4/2021 12:28 PM, Michael Niedermayer wrote: > > On Fri, Sep 03, 2021 at 03:45:55PM -0300, James Almer wrote: > > > On 9/3/2021 3:39 PM, Michael Niedermayer wrote: > > > > Fixes: Timeout (56sec -> 15sec) > > > > Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ > > > > 1 file changed, 32 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c > > > > index e5105ecc58..e91c554781 100644 > > > > --- a/libavcodec/mpegutils.c > > > > +++ b/libavcodec/mpegutils.c > > > > @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > av_freep(&mvs); > > > > } > > > > - > > > > > > Stray change. > > > > > > > /* TODO: export all the following to make them accessible for users (and filters) */ > > > > if (avctx->hwaccel || !mbtype_table) > > > > return; > > > > @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ > > > > if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { > > > > int x,y; > > > > +#define MB_STRING_SIZE 6 > > > > + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); > > > > + if (!mbstring) > > > > + return; > > > > av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", > > > > av_get_picture_type_char(pict->pict_type)); > > > > for (y = 0; y < mb_height; y++) { > > > > + char *mbs = mbstring; > > > > for (x = 0; x < mb_width; x++) { > > > > + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); > > > > if (avctx->debug & FF_DEBUG_SKIP) { > > > > int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; > > > > if (count > 9) > > > > count = 9; > > > > - av_log(avctx, AV_LOG_DEBUG, "%1d", count); > > > > + *mbs++ = '0' + count; > > > > > > This is ugly and not very obvious at first glance. Can you use lavu's bprint > > > or something like that instead? > > > > the whole point of the patch is to eliminate the string formating overhead. > > (which surprisingly is significant) > > so as long as we have a "%d" style format in there, it at least feels like we > > arent doing a good job at eliminating it > > Is the string formatting the problem, or the constant calls to av_log()? > > av_bprintf() would do the string formatting, but append each character into > a buffer you can then use to call av_log() once at the end. > Something like > > av_bprint_init(&bp, MB_STRING_SIZE * mb_width + 1, UINT_MAX); > [...] > av_bprintf(&bp, "%1d", count); > av_bprint_chars(&bp, 'P', 1); > [...] > av_bprint_finalize(&bp, &buf); > av_log(avctx, AV_LOG_DEBUG, "%s\n" buf); > av_free(buf); > > > > > would moving this in a seperate function resolve this concern too ? > > If the above doesn't perform well (Or you don't feel like trying it), then > an inline function or macro should be fine. Where do you want me to use a macro ? just the '0' + count; or for other things too? thx [...]
diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c index e5105ecc58..e91c554781 100644 --- a/libavcodec/mpegutils.c +++ b/libavcodec/mpegutils.c @@ -187,7 +187,6 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ av_freep(&mvs); } - /* TODO: export all the following to make them accessible for users (and filters) */ if (avctx->hwaccel || !mbtype_table) return; @@ -195,71 +194,80 @@ void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_ if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) { int x,y; +#define MB_STRING_SIZE 6 + char *mbstring = av_malloc(MB_STRING_SIZE * mb_width + 1); + if (!mbstring) + return; av_log(avctx, AV_LOG_DEBUG, "New frame, type: %c\n", av_get_picture_type_char(pict->pict_type)); for (y = 0; y < mb_height; y++) { + char *mbs = mbstring; for (x = 0; x < mb_width; x++) { + av_assert0(mbs - mbstring <= MB_STRING_SIZE * x); if (avctx->debug & FF_DEBUG_SKIP) { int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0; if (count > 9) count = 9; - av_log(avctx, AV_LOG_DEBUG, "%1d", count); + *mbs++ = '0' + count; } if (avctx->debug & FF_DEBUG_QP) { - av_log(avctx, AV_LOG_DEBUG, "%2d", - qscale_table[x + y * mb_stride]); + int q = qscale_table[x + y * mb_stride]; + *mbs++ = '0' + q/10; + *mbs++ = '0' + q%10; } if (avctx->debug & FF_DEBUG_MB_TYPE) { int mb_type = mbtype_table[x + y * mb_stride]; // Type & MV direction if (IS_PCM(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "P"); + *mbs++ = 'P'; else if (IS_INTRA(mb_type) && IS_ACPRED(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "A"); + *mbs++ = 'A'; else if (IS_INTRA4x4(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "i"); + *mbs++ = 'i'; else if (IS_INTRA16x16(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "I"); + *mbs++ = 'I'; else if (IS_DIRECT(mb_type) && IS_SKIP(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "d"); + *mbs++ = 'd'; else if (IS_DIRECT(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "D"); + *mbs++ = 'D'; else if (IS_GMC(mb_type) && IS_SKIP(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "g"); + *mbs++ = 'g'; else if (IS_GMC(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "G"); + *mbs++ = 'G'; else if (IS_SKIP(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "S"); + *mbs++ = 'S'; else if (!USES_LIST(mb_type, 1)) - av_log(avctx, AV_LOG_DEBUG, ">"); + *mbs++ = '>'; else if (!USES_LIST(mb_type, 0)) - av_log(avctx, AV_LOG_DEBUG, "<"); + *mbs++ = '<'; else { av_assert2(USES_LIST(mb_type, 0) && USES_LIST(mb_type, 1)); - av_log(avctx, AV_LOG_DEBUG, "X"); + *mbs++ = 'X'; } // segmentation if (IS_8X8(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "+"); + *mbs++ = '+'; else if (IS_16X8(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "-"); + *mbs++ = '-'; else if (IS_8X16(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "|"); + *mbs++ = '|'; else if (IS_INTRA(mb_type) || IS_16X16(mb_type)) - av_log(avctx, AV_LOG_DEBUG, " "); + *mbs++ = ' '; else - av_log(avctx, AV_LOG_DEBUG, "?"); + *mbs++ = '?'; if (IS_INTERLACED(mb_type)) - av_log(avctx, AV_LOG_DEBUG, "="); + *mbs++ = '='; else - av_log(avctx, AV_LOG_DEBUG, " "); + *mbs++ = ' '; } } - av_log(avctx, AV_LOG_DEBUG, "\n"); + *mbs++ = 0; + av_log(avctx, AV_LOG_DEBUG, "%s\n", mbstring); } + av_freep(&mbstring); } }
Fixes: Timeout (56sec -> 15sec) Fixes: 37141/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG4_fuzzer-6192122867875840 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/mpegutils.c | 56 ++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 24 deletions(-)