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

[James Almer]

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.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>