[FFmpeg-devel,v3,1/3] avcodec/v4l2_m2m_enc: Enable frame level rate control by default

From: Andriy Gelman <andriy.gelman@gmail.com>

Without this setting, bitrate and qmin/qmax options have no
affect on the s5p-mfc hardware encoder.

Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
---
 libavcodec/v4l2_m2m_enc.c | 1 +
 1 file changed, 1 insertion(+)

On 16/02/2020 19:31, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> Without this setting, bitrate and qmin/qmax options have no
> affect on the s5p-mfc hardware encoder.
> 
> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> ---
>  libavcodec/v4l2_m2m_enc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index 98b9dfc2c0b..94373b3b6e7 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -177,6 +177,7 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
>      /* set ext ctrls */
>      v4l2_set_ext_ctrl(s, MPEG_CID(HEADER_MODE), MPEG_VIDEO(HEADER_MODE_SEPARATE), "header mode");
>      v4l2_set_ext_ctrl(s, MPEG_CID(BITRATE) , avctx->bit_rate, "bit rate");
> +    v4l2_set_ext_ctrl(s, MPEG_CID(FRAME_RC_ENABLE), 1, "frame level rate control");
>      v4l2_set_ext_ctrl(s, MPEG_CID(GOP_SIZE), avctx->gop_size,"gop size");
>  
>      av_log(avctx, AV_LOG_DEBUG,
> 

Can you check the behaviour with other drivers here?

If they do use the bitrate option even when this was not set (presumably at least one does, because that must have been tested somewhere) then they probably aren't supported, and it looks like the user will get an unhelpful error from trying to send this option to a driver which doesn't support it.  (A trivial grep suggests that only s5p-mfc and mtk-vcodec support it, but I might be missing something.)

Thanks,

- Mark

On Sun, 16. Feb 20:27, Mark Thompson wrote:
> On 16/02/2020 19:31, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > Without this setting, bitrate and qmin/qmax options have no
> > affect on the s5p-mfc hardware encoder.
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> > ---
> >  libavcodec/v4l2_m2m_enc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> > index 98b9dfc2c0b..94373b3b6e7 100644
> > --- a/libavcodec/v4l2_m2m_enc.c
> > +++ b/libavcodec/v4l2_m2m_enc.c
> > @@ -177,6 +177,7 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
> >      /* set ext ctrls */
> >      v4l2_set_ext_ctrl(s, MPEG_CID(HEADER_MODE), MPEG_VIDEO(HEADER_MODE_SEPARATE), "header mode");
> >      v4l2_set_ext_ctrl(s, MPEG_CID(BITRATE) , avctx->bit_rate, "bit rate");
> > +    v4l2_set_ext_ctrl(s, MPEG_CID(FRAME_RC_ENABLE), 1, "frame level rate control");
> >      v4l2_set_ext_ctrl(s, MPEG_CID(GOP_SIZE), avctx->gop_size,"gop size");
> >  
> >      av_log(avctx, AV_LOG_DEBUG,
> > 

> 
> Can you check the behaviour with other drivers here?
> 
> If they do use the bitrate option even when this was not set (presumably at least one does, because that must have been tested somewhere) then they probably aren't supported, and it looks like the user will get an unhelpful error from trying to send this option to a driver which doesn't support it.  (A trivial grep suggests that only s5p-mfc and mtk-vcodec support it, but I might be missing something.)
> 

I tested on the RPI4, and the flag is not supported. Actually, only a few parameters are used and we end up generating lots of error messages shown below.

Maybe the best strategy is to check whether the parameter exists in v4l2_set_ext_ctrl before trying to set it? 

[h264_v4l2m2m @ 0x30beec0] Using device /dev/video11
[h264_v4l2m2m @ 0x30beec0] driver 'bcm2835-codec' on card 'bcm2835-codec-encode' in mplane mode
[h264_v4l2m2m @ 0x30beec0] requesting formats: output=YU12 capture=H264
[h264_v4l2m2m @ 0x30beec0] Failed to set number of B-frames: Invalid argument
[h264_v4l2m2m @ 0x30beec0] Failed to get number of B-frames
[h264_v4l2m2m @ 0x30beec0] Failed to set header mode: Invalid argument
[h264_v4l2m2m @ 0x30beec0] Failed to set frame level rate control: Invalid argument
[h264_v4l2m2m @ 0x30beec0] Failed to set gop size: Invalid argument
[h264_v4l2m2m @ 0x30beec0] h264 profile not found
[h264_v4l2m2m @ 0x30beec0] Failed to set minimum video quantizer scale: Invalid argument
[h264_v4l2m2m @ 0x30beec0] Failed to set maximum video quantizer scale: Invalid argument

Thanks

On 16/02/2020 20:41, Andriy Gelman wrote:
> On Sun, 16. Feb 20:27, Mark Thompson wrote:
>> On 16/02/2020 19:31, Andriy Gelman wrote:
>>> From: Andriy Gelman <andriy.gelman@gmail.com>
>>>
>>> Without this setting, bitrate and qmin/qmax options have no
>>> affect on the s5p-mfc hardware encoder.
>>>
>>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
>>> ---
>>>  libavcodec/v4l2_m2m_enc.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
>>> index 98b9dfc2c0b..94373b3b6e7 100644
>>> --- a/libavcodec/v4l2_m2m_enc.c
>>> +++ b/libavcodec/v4l2_m2m_enc.c
>>> @@ -177,6 +177,7 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
>>>      /* set ext ctrls */
>>>      v4l2_set_ext_ctrl(s, MPEG_CID(HEADER_MODE), MPEG_VIDEO(HEADER_MODE_SEPARATE), "header mode");
>>>      v4l2_set_ext_ctrl(s, MPEG_CID(BITRATE) , avctx->bit_rate, "bit rate");
>>> +    v4l2_set_ext_ctrl(s, MPEG_CID(FRAME_RC_ENABLE), 1, "frame level rate control");
>>>      v4l2_set_ext_ctrl(s, MPEG_CID(GOP_SIZE), avctx->gop_size,"gop size");
>>>  
>>>      av_log(avctx, AV_LOG_DEBUG,
>>>
> 
>>
>> Can you check the behaviour with other drivers here?
>>
>> If they do use the bitrate option even when this was not set (presumably at least one does, because that must have been tested somewhere) then they probably aren't supported, and it looks like the user will get an unhelpful error from trying to send this option to a driver which doesn't support it.  (A trivial grep suggests that only s5p-mfc and mtk-vcodec support it, but I might be missing something.)
>>
> 
> I tested on the RPI4, and the flag is not supported. Actually, only a few parameters are used and we end up generating lots of error messages shown below.
> 
> Maybe the best strategy is to check whether the parameter exists in v4l2_set_ext_ctrl before trying to set it? 

That sounds better to me.

> [h264_v4l2m2m @ 0x30beec0] Using device /dev/video11
> [h264_v4l2m2m @ 0x30beec0] driver 'bcm2835-codec' on card 'bcm2835-codec-encode' in mplane mode
> [h264_v4l2m2m @ 0x30beec0] requesting formats: output=YU12 capture=H264
> [h264_v4l2m2m @ 0x30beec0] Failed to set number of B-frames: Invalid argument
> [h264_v4l2m2m @ 0x30beec0] Failed to get number of B-frames
> [h264_v4l2m2m @ 0x30beec0] Failed to set header mode: Invalid argument
> [h264_v4l2m2m @ 0x30beec0] Failed to set frame level rate control: Invalid argument
> [h264_v4l2m2m @ 0x30beec0] Failed to set gop size: Invalid argument
> [h264_v4l2m2m @ 0x30beec0] h264 profile not found
> [h264_v4l2m2m @ 0x30beec0] Failed to set minimum video quantizer scale: Invalid argument
> [h264_v4l2m2m @ 0x30beec0] Failed to set maximum video quantizer scale: Invalid argument

The treatment probably has to be dependent on the option:

The B-frames one is just a max, so it can be safely ignored if not supported.

The header mode kindof needs to be set, but I guess if the encoder works at all then it must always be leaving the headers inline so likely ignorable.

Frame level RC must default to the right thing if bitrate works at all.

GOP size could cause nasty problems if it's wrong, so that seems worth warning about.

Profile similarly, but only if the user actually set a profile (avctx->profile is not FF_PROFILE_UNKNOWN).

Thanks,

- Mark