[FFmpeg-devel] [PATCH v15 1/2] lavc/svt_hevc: add libsvt hevc encoder wrapper

[Sun, Jing A]

-----Original Message-----
From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf Of Li, Zhong
Sent: Wednesday, July 31, 2019 11:33 AM
To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v15 1/2] lavc/svt_hevc: add libsvt hevc encoder wrapper
 
> > > > +AVCodec ff_libsvt_hevc_encoder = {
> > > > +    .name           = "libsvt_hevc",
> > > > +    .long_name      =
> > NULL_IF_CONFIG_SMALL("SVT-HEVC(Scalable
> > > Video Technology for HEVC) encoder"),
> > > > +    .priv_data_size = sizeof(SvtContext),
> > > > +    .type           = AVMEDIA_TYPE_VIDEO,
> > > > +    .id             = AV_CODEC_ID_HEVC,
> > > > +    .init           = eb_enc_init,
> > > > +    .encode2        = eb_encode_frame,
> > > > +    .close          = eb_enc_close,
> > > > +    .capabilities   = AV_CODEC_CAP_DELAY |
> > > AV_CODEC_CAP_AUTO_THREADS,
> > >
> > > The code don't support to configure thread_count, so I think you'll 
> > > get the same result without AV_CODEC_CAP_AUTO_THREADS.
> > 
> > > This was pointed out by Mark Thompson on patch V4.
> > > It is a problem how comment can be well addressed and avoid to be
> > pointed out again and again.
> > 
> > I got started based on V5 and V6 is my first submission. Thanks for 
> > showing me the information. I am looking into it.
>It was also mentioned in svt-av1 patch https://patchwork.ffmpeg.org/patch/13912/：
>* Expose threads setting since AV_CODEC_CAP_AUTO_THREADS declared _______________________________________________



I checked out the history in https://patchwork.ffmpeg.org/patch/11347/:

Mark's remarks:
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
Threads aren't actually mentioned at all above.  Does that mean the encoder will always make as many threads as it wants, and the user has no separate control?

And from the macro definition:
/**
 * Codec supports avctx->thread_count == 0 (auto).
 */
#define AV_CODEC_CAP_AUTO_THREADS        (1 << 15)

static void validate_thread_parameters(AVCodecContext *avctx)
{
    ...
    } else if (!(avctx->codec->capabilities & AV_CODEC_CAP_AUTO_THREADS)) {
        avctx->thread_count       = 1;
        avctx->active_thread_type = 0;
    }
    ...
}

I think it's right to declare the codec with that capability as long as the codec indeed supports the auto thread count. It's not wrong that we have declared that. And to Mark's question, the answer is "Yes, the SVT HEVC encoder always makes as many threads as it wants". It checks the CPU cores and decides the count of working threads based on the number of cores to ensure the best performance, and by now users have no separate control on it.

We have been considering if we shall add such a controlling interface to SVT HEVC lib and how. The code lines of handling avctx->thread_count in this avcodec is possible to get added in the future. Right now, adding an empty interface without implementation supported by SVT HEVC lib makes no sense.
 
Regards,
Sun, Jing