[FFmpeg-devel] [PATCH 3/4] avcodec/mpegutils: consolidate single byte av_log()

[Michael Niedermayer]

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 at 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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20211007/98cfc22f/attachment.sig>