diff mbox series

[FFmpeg-devel,v2] avcodec/v4l2_m2m_enc: Support changing qmin/qmax

Message ID 20200119195402.9983-1-andriy.gelman@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/v4l2_m2m_enc: Support changing qmin/qmax
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andriy Gelman Jan. 19, 2020, 7:54 p.m. UTC
From: Andriy Gelman <andriy.gelman@gmail.com>

Hard coded parameters for qmin and qmax are currently used to initialize
v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
are set.

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

Comments

Gyan Doshi Jan. 23, 2020, 12:39 p.m. UTC | #1
On 20-01-2020 01:24 am, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
>
> Hard coded parameters for qmin and qmax are currently used to initialize
> v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
> are set.
>
> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> ---
>   libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index 8059e3bb48f..318be0d3379 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -31,10 +31,25 @@
>   #include "v4l2_context.h"
>   #include "v4l2_m2m.h"
>   #include "v4l2_fmt.h"
> +#include "internal.h"
>   
>   #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
>   #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
>   
> +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
> +    do {                                                    \
> +        if ((in) < (min_val)) {                             \
> +            av_log((logctx), AV_LOG_WARNING,                \
> +                   "Adjusted: " name " (%d)\n", (min_val)); \
> +            in = min_val;                                   \
> +        }                                                   \
> +        if ((in) > (max_val)) {                             \
> +            av_log((logctx), AV_LOG_WARNING,                \
> +                   "Adjusted: " name " (%d)\n", (max_val)); \
> +            (in) = (max_val);                               \
> +        }                                                   \
> +    } while (0)
> +

I was thinking to just use the appropriate av_clip variant.

>   static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
>   {
>       struct v4l2_streamparm parm = { 0 };
> @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
>           return 0;
>       }
>   
> -    if (qmin != avctx->qmin || qmax != avctx->qmax)
> -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
> +    if (avctx->qmin >= 0) {
> +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
> +        qmin = avctx->qmin;
> +    }
> +
> +    if (avctx->qmax >= 0) {
> +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
> +        qmax = avctx->qmax;
> +    }
>   
>       v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
>       v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
> @@ -349,6 +371,12 @@ static const AVOption options[] = {
>       { NULL },
>   };
>   
> +static const AVCodecDefault v4l2_m2m_defaults[] = {
> +    { "qmin", "-1" },
> +    { "qmax", "-1" },
> +    { NULL },
> +};
> +
>   #define M2MENC_CLASS(NAME) \
>       static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
>           .class_name = #NAME "_v4l2m2m_encoder", \
> @@ -370,6 +398,7 @@ static const AVOption options[] = {
>           .send_frame     = v4l2_send_frame, \
>           .receive_packet = v4l2_receive_packet, \
>           .close          = v4l2_encode_close, \
> +        .defaults       = v4l2_m2m_defaults, \
>           .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
>           .wrapper_name   = "v4l2m2m", \
>       };

Gyan
Mark Thompson Feb. 1, 2020, 10:38 p.m. UTC | #2
On 19/01/2020 19:54, Andriy Gelman wrote:
> From: Andriy Gelman <andriy.gelman@gmail.com>
> 
> Hard coded parameters for qmin and qmax are currently used to initialize
> v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
> are set.
> 
> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> ---
>  libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> index 8059e3bb48f..318be0d3379 100644
> --- a/libavcodec/v4l2_m2m_enc.c
> +++ b/libavcodec/v4l2_m2m_enc.c
> @@ -31,10 +31,25 @@
>  #include "v4l2_context.h"
>  #include "v4l2_m2m.h"
>  #include "v4l2_fmt.h"
> +#include "internal.h"
>  
>  #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
>  #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
>  
> +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
> +    do {                                                    \
> +        if ((in) < (min_val)) {                             \
> +            av_log((logctx), AV_LOG_WARNING,                \
> +                   "Adjusted: " name " (%d)\n", (min_val)); \
> +            in = min_val;                                   \
> +        }                                                   \
> +        if ((in) > (max_val)) {                             \
> +            av_log((logctx), AV_LOG_WARNING,                \
> +                   "Adjusted: " name " (%d)\n", (max_val)); \
> +            (in) = (max_val);                               \
> +        }                                                   \
> +    } while (0)
> +
>  static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
>  {
>      struct v4l2_streamparm parm = { 0 };
> @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
>          return 0;
>      }
>  
> -    if (qmin != avctx->qmin || qmax != avctx->qmax)
> -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
> +    if (avctx->qmin >= 0) {
> +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
> +        qmin = avctx->qmin;
> +    }
> +
> +    if (avctx->qmax >= 0) {
> +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
> +        qmax = avctx->qmax;
> +    }
>  
>      v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
>      v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
> @@ -349,6 +371,12 @@ static const AVOption options[] = {
>      { NULL },
>  };
>  
> +static const AVCodecDefault v4l2_m2m_defaults[] = {
> +    { "qmin", "-1" },
> +    { "qmax", "-1" },
> +    { NULL },
> +};
> +
>  #define M2MENC_CLASS(NAME) \
>      static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
>          .class_name = #NAME "_v4l2m2m_encoder", \
> @@ -370,6 +398,7 @@ static const AVOption options[] = {
>          .send_frame     = v4l2_send_frame, \
>          .receive_packet = v4l2_receive_packet, \
>          .close          = v4l2_encode_close, \
> +        .defaults       = v4l2_m2m_defaults, \
>          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
>          .wrapper_name   = "v4l2m2m", \
>      };
> 

Can we avoid some of the clumsiness around clipping twice in different ways by querying the quantiser values from the encode device here?

E.g. on s5p-mfc (exynos) I can see:

          h264_minimum_qp_value 0x00990a61 (int)    : min=0 max=51 step=1 default=1 value=1
          h264_maximum_qp_value 0x00990a62 (int)    : min=0 max=51 step=1 default=51 value=51

which matches the expected values for 8-bit H.264, but then for VP8 there is:

           vpx_minimum_qp_value 0x00990afb (int)    : min=0 max=11 step=1 default=0 value=0
           vpx_maximum_qp_value 0x00990afc (int)    : min=0 max=127 step=1 default=127 value=127

which is going to pretty much entirely stop qmin from doing anything useful, and it would be nice to diagnose that.

- Mark
Andriy Gelman Feb. 2, 2020, 1:33 a.m. UTC | #3
On Sat, 01. Feb 22:38, Mark Thompson wrote:
> On 19/01/2020 19:54, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > Hard coded parameters for qmin and qmax are currently used to initialize
> > v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
> > are set.
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> > ---
> >  libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
> >  1 file changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> > index 8059e3bb48f..318be0d3379 100644
> > --- a/libavcodec/v4l2_m2m_enc.c
> > +++ b/libavcodec/v4l2_m2m_enc.c
> > @@ -31,10 +31,25 @@
> >  #include "v4l2_context.h"
> >  #include "v4l2_m2m.h"
> >  #include "v4l2_fmt.h"
> > +#include "internal.h"
> >  
> >  #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
> >  #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
> >  
> > +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
> > +    do {                                                    \
> > +        if ((in) < (min_val)) {                             \
> > +            av_log((logctx), AV_LOG_WARNING,                \
> > +                   "Adjusted: " name " (%d)\n", (min_val)); \
> > +            in = min_val;                                   \
> > +        }                                                   \
> > +        if ((in) > (max_val)) {                             \
> > +            av_log((logctx), AV_LOG_WARNING,                \
> > +                   "Adjusted: " name " (%d)\n", (max_val)); \
> > +            (in) = (max_val);                               \
> > +        }                                                   \
> > +    } while (0)
> > +
> >  static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
> >  {
> >      struct v4l2_streamparm parm = { 0 };
> > @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
> >          return 0;
> >      }
> >  
> > -    if (qmin != avctx->qmin || qmax != avctx->qmax)
> > -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
> > +    if (avctx->qmin >= 0) {
> > +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
> > +        qmin = avctx->qmin;
> > +    }
> > +
> > +    if (avctx->qmax >= 0) {
> > +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
> > +        qmax = avctx->qmax;
> > +    }
> >  
> >      v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
> >      v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
> > @@ -349,6 +371,12 @@ static const AVOption options[] = {
> >      { NULL },
> >  };
> >  
> > +static const AVCodecDefault v4l2_m2m_defaults[] = {
> > +    { "qmin", "-1" },
> > +    { "qmax", "-1" },
> > +    { NULL },
> > +};
> > +
> >  #define M2MENC_CLASS(NAME) \
> >      static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
> >          .class_name = #NAME "_v4l2m2m_encoder", \
> > @@ -370,6 +398,7 @@ static const AVOption options[] = {
> >          .send_frame     = v4l2_send_frame, \
> >          .receive_packet = v4l2_receive_packet, \
> >          .close          = v4l2_encode_close, \
> > +        .defaults       = v4l2_m2m_defaults, \
> >          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
> >          .wrapper_name   = "v4l2m2m", \
> >      };
> > 

> 
> Can we avoid some of the clumsiness around clipping twice in different ways by querying the quantiser values from the encode device here?
> 

thanks, I'll investigate.

> E.g. on s5p-mfc (exynos) I can see:
> 
>           h264_minimum_qp_value 0x00990a61 (int)    : min=0 max=51 step=1 default=1 value=1
>           h264_maximum_qp_value 0x00990a62 (int)    : min=0 max=51 step=1 default=51 value=51
> 
> which matches the expected values for 8-bit H.264, but then for VP8 there is:
> 
>            vpx_minimum_qp_value 0x00990afb (int)    : min=0 max=11 step=1 default=0 value=0
>            vpx_maximum_qp_value 0x00990afc (int)    : min=0 max=127 step=1 default=127 value=127
> 
> which is going to pretty much entirely stop qmin from doing anything useful, and it would be nice to diagnose that.

The max for vpx_maximum_qp_value has been bothering me. The usual qp range for vp8/9 is [0,63]. 

I checked today with a dev on #v4l and he told me that only 6 bits are used in
the driver so it's probably a bug. I'll look into this further. 

On a related note, I actually haven't been able to play a stream that's been
compressed with vp8/9 on the s5p-mfc (odroid xu4 for me). Have you had any
luck? 

Andriy
Mark Thompson Feb. 2, 2020, 12:30 p.m. UTC | #4
On 02/02/2020 01:33, Andriy Gelman wrote:
> On Sat, 01. Feb 22:38, Mark Thompson wrote:
>> On 19/01/2020 19:54, Andriy Gelman wrote:
>>> From: Andriy Gelman <andriy.gelman@gmail.com>
>>>
>>> Hard coded parameters for qmin and qmax are currently used to initialize
>>> v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
>>> are set.
>>>
>>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
>>> ---
>>>  libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
>>>  1 file changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
>>> index 8059e3bb48f..318be0d3379 100644
>>> --- a/libavcodec/v4l2_m2m_enc.c
>>> +++ b/libavcodec/v4l2_m2m_enc.c
>>> @@ -31,10 +31,25 @@
>>>  #include "v4l2_context.h"
>>>  #include "v4l2_m2m.h"
>>>  #include "v4l2_fmt.h"
>>> +#include "internal.h"
>>>  
>>>  #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
>>>  #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
>>>  
>>> +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
>>> +    do {                                                    \
>>> +        if ((in) < (min_val)) {                             \
>>> +            av_log((logctx), AV_LOG_WARNING,                \
>>> +                   "Adjusted: " name " (%d)\n", (min_val)); \
>>> +            in = min_val;                                   \
>>> +        }                                                   \
>>> +        if ((in) > (max_val)) {                             \
>>> +            av_log((logctx), AV_LOG_WARNING,                \
>>> +                   "Adjusted: " name " (%d)\n", (max_val)); \
>>> +            (in) = (max_val);                               \
>>> +        }                                                   \
>>> +    } while (0)
>>> +
>>>  static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
>>>  {
>>>      struct v4l2_streamparm parm = { 0 };
>>> @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
>>>          return 0;
>>>      }
>>>  
>>> -    if (qmin != avctx->qmin || qmax != avctx->qmax)
>>> -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
>>> +    if (avctx->qmin >= 0) {
>>> +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
>>> +        qmin = avctx->qmin;
>>> +    }
>>> +
>>> +    if (avctx->qmax >= 0) {
>>> +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
>>> +        qmax = avctx->qmax;
>>> +    }
>>>  
>>>      v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
>>>      v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
>>> @@ -349,6 +371,12 @@ static const AVOption options[] = {
>>>      { NULL },
>>>  };
>>>  
>>> +static const AVCodecDefault v4l2_m2m_defaults[] = {
>>> +    { "qmin", "-1" },
>>> +    { "qmax", "-1" },
>>> +    { NULL },
>>> +};
>>> +
>>>  #define M2MENC_CLASS(NAME) \
>>>      static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
>>>          .class_name = #NAME "_v4l2m2m_encoder", \
>>> @@ -370,6 +398,7 @@ static const AVOption options[] = {
>>>          .send_frame     = v4l2_send_frame, \
>>>          .receive_packet = v4l2_receive_packet, \
>>>          .close          = v4l2_encode_close, \
>>> +        .defaults       = v4l2_m2m_defaults, \
>>>          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
>>>          .wrapper_name   = "v4l2m2m", \
>>>      };
>>>
> 
>>
>> Can we avoid some of the clumsiness around clipping twice in different ways by querying the quantiser values from the encode device here?
>>
> 
> thanks, I'll investigate.
> 
>> E.g. on s5p-mfc (exynos) I can see:
>>
>>           h264_minimum_qp_value 0x00990a61 (int)    : min=0 max=51 step=1 default=1 value=1
>>           h264_maximum_qp_value 0x00990a62 (int)    : min=0 max=51 step=1 default=51 value=51
>>
>> which matches the expected values for 8-bit H.264, but then for VP8 there is:
>>
>>            vpx_minimum_qp_value 0x00990afb (int)    : min=0 max=11 step=1 default=0 value=0
>>            vpx_maximum_qp_value 0x00990afc (int)    : min=0 max=127 step=1 default=127 value=127
>>
>> which is going to pretty much entirely stop qmin from doing anything useful, and it would be nice to diagnose that.
> 
> The max for vpx_maximum_qp_value has been bothering me. The usual qp range for vp8/9 is [0,63].

Ha, this is a fun subject.

Quant range for VP8 is a 7-bit value, so [0..127].  (<https://tools.ietf.org/html/rfc6386#section-9.6>.)

Quant range for VP9/AV1 is an 8-bit value, so [0..255].  (VP9, see §6.2.9; AV1, see §5.9.12.)

The libvpx/libaom software encoders present an external interface which compresses the range to 6 bits [0..63] (I think purely so that it looks more like libx264), but then immediately remap to the actual codec range before doing anything with the result.  Other encoders may or may not have followed that and used the compressed range, so you're pretty much stuck with trying it or looking at the source code of a given encoder to find out which one they used.

For V4L2 I can't find any official definition of which one is being used, and there are multiple different encoders behind it.

Of the three drivers in mainline Linux, two (s5p-mfc and venus) advertise that they use the codec range:

<https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/media/platform/qcom/venus/venc_ctrls.c?h=v5.4.17#n339>
<https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c?h=v5.4.17#n650>

while the other (mtk-vcodec) doesn't support any quantiser controls at all (I guess it only does bitrate targets).

> I checked today with a dev on #v4l and he told me that only 6 bits are used in
> the driver so it's probably a bug. I'll look into this further. 

If the s5p-mfc driver actually uses the libvpx range rather than the codec range, they should probably talk to the venus people to get agreement on what range the controls are using.  If venus does use the codec range but the s5p-mfc firmware blob wants the libvpx range then they would be better off putting a remapping table in their driver than changing their range to not match.

> On a related note, I actually haven't been able to play a stream that's been
> compressed with vp8/9 on the s5p-mfc (odroid xu4 for me). Have you had any
> luck? 

I haven't seen it make a valid stream, but I've only ever done token testing with it to observe that.  I assume it does work on venus, since that was what the Linaro people were originally testing on.

- Mark
Andriy Gelman Feb. 2, 2020, 10:14 p.m. UTC | #5
On Sun, 02. Feb 12:30, Mark Thompson wrote:
> On 02/02/2020 01:33, Andriy Gelman wrote:
> > On Sat, 01. Feb 22:38, Mark Thompson wrote:
> >> On 19/01/2020 19:54, Andriy Gelman wrote:
> >>> From: Andriy Gelman <andriy.gelman@gmail.com>
> >>>
> >>> Hard coded parameters for qmin and qmax are currently used to initialize
> >>> v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
> >>> are set.
> >>>
> >>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> >>> ---
> >>>  libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
> >>>  1 file changed, 31 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> >>> index 8059e3bb48f..318be0d3379 100644
> >>> --- a/libavcodec/v4l2_m2m_enc.c
> >>> +++ b/libavcodec/v4l2_m2m_enc.c
> >>> @@ -31,10 +31,25 @@
> >>>  #include "v4l2_context.h"
> >>>  #include "v4l2_m2m.h"
> >>>  #include "v4l2_fmt.h"
> >>> +#include "internal.h"
> >>>  
> >>>  #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
> >>>  #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
> >>>  
> >>> +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
> >>> +    do {                                                    \
> >>> +        if ((in) < (min_val)) {                             \
> >>> +            av_log((logctx), AV_LOG_WARNING,                \
> >>> +                   "Adjusted: " name " (%d)\n", (min_val)); \
> >>> +            in = min_val;                                   \
> >>> +        }                                                   \
> >>> +        if ((in) > (max_val)) {                             \
> >>> +            av_log((logctx), AV_LOG_WARNING,                \
> >>> +                   "Adjusted: " name " (%d)\n", (max_val)); \
> >>> +            (in) = (max_val);                               \
> >>> +        }                                                   \
> >>> +    } while (0)
> >>> +
> >>>  static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
> >>>  {
> >>>      struct v4l2_streamparm parm = { 0 };
> >>> @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
> >>>          return 0;
> >>>      }
> >>>  
> >>> -    if (qmin != avctx->qmin || qmax != avctx->qmax)
> >>> -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
> >>> +    if (avctx->qmin >= 0) {
> >>> +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
> >>> +        qmin = avctx->qmin;
> >>> +    }
> >>> +
> >>> +    if (avctx->qmax >= 0) {
> >>> +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
> >>> +        qmax = avctx->qmax;
> >>> +    }
> >>>  
> >>>      v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
> >>>      v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
> >>> @@ -349,6 +371,12 @@ static const AVOption options[] = {
> >>>      { NULL },
> >>>  };
> >>>  
> >>> +static const AVCodecDefault v4l2_m2m_defaults[] = {
> >>> +    { "qmin", "-1" },
> >>> +    { "qmax", "-1" },
> >>> +    { NULL },
> >>> +};
> >>> +
> >>>  #define M2MENC_CLASS(NAME) \
> >>>      static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
> >>>          .class_name = #NAME "_v4l2m2m_encoder", \
> >>> @@ -370,6 +398,7 @@ static const AVOption options[] = {
> >>>          .send_frame     = v4l2_send_frame, \
> >>>          .receive_packet = v4l2_receive_packet, \
> >>>          .close          = v4l2_encode_close, \
> >>> +        .defaults       = v4l2_m2m_defaults, \
> >>>          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
> >>>          .wrapper_name   = "v4l2m2m", \
> >>>      };
> >>>
> > 
> >>
> >> Can we avoid some of the clumsiness around clipping twice in different ways by querying the quantiser values from the encode device here?
> >>
> > 
> > thanks, I'll investigate.
> > 
> >> E.g. on s5p-mfc (exynos) I can see:
> >>
> >>           h264_minimum_qp_value 0x00990a61 (int)    : min=0 max=51 step=1 default=1 value=1
> >>           h264_maximum_qp_value 0x00990a62 (int)    : min=0 max=51 step=1 default=51 value=51
> >>
> >> which matches the expected values for 8-bit H.264, but then for VP8 there is:
> >>
> >>            vpx_minimum_qp_value 0x00990afb (int)    : min=0 max=11 step=1 default=0 value=0
> >>            vpx_maximum_qp_value 0x00990afc (int)    : min=0 max=127 step=1 default=127 value=127
> >>
> >> which is going to pretty much entirely stop qmin from doing anything useful, and it would be nice to diagnose that.
> > 
> > The max for vpx_maximum_qp_value has been bothering me. The usual qp range for vp8/9 is [0,63].

> 
> Ha, this is a fun subject.
> 
> Quant range for VP8 is a 7-bit value, so [0..127].  (<https://tools.ietf.org/html/rfc6386#section-9.6>.)
> 
> Quant range for VP9/AV1 is an 8-bit value, so [0..255].  (VP9, see §6.2.9; AV1, see §5.9.12.)
> 
> The libvpx/libaom software encoders present an external interface which compresses the range to 6 bits [0..63] (I think purely so that it looks more like libx264), but then immediately remap to the actual codec range before doing anything with the result.  Other encoders may or may not have followed that and used the compressed range, so you're pretty much stuck with trying it or looking at the source code of a given encoder to find out which one they used.
> 
> For V4L2 I can't find any official definition of which one is being used, and there are multiple different encoders behind it.
> 
> Of the three drivers in mainline Linux, two (s5p-mfc and venus) advertise that they use the codec range:
> 
> <https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/media/platform/qcom/venus/venc_ctrls.c?h=v5.4.17#n339>
> <https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c?h=v5.4.17#n650>
> 
> while the other (mtk-vcodec) doesn't support any quantiser controls at all (I guess it only does bitrate targets).

thanks Mark, that's very interesting.

> 
> > I checked today with a dev on #v4l and he told me that only 6 bits are used in
> > the driver so it's probably a bug. I'll look into this further. 
> 
> If the s5p-mfc driver actually uses the libvpx range rather than the codec range, they should probably talk to the venus people to get agreement on what range the controls are using.  If venus does use the codec range but the s5p-mfc firmware blob wants the libvpx range then they would be better off putting a remapping table in their driver than changing their range to not match.
> 
> > On a related note, I actually haven't been able to play a stream that's been
> > compressed with vp8/9 on the s5p-mfc (odroid xu4 for me). Have you had any
> > luck? 

> 
> I haven't seen it make a valid stream, but I've only ever done token testing with it to observe that.  I assume it does work on venus, since that was what the Linaro people were originally testing on.

I'll try to get hold of a venus board to test.
Andriy Gelman Feb. 16, 2020, 7:28 p.m. UTC | #6
On Sat, 01. Feb 22:38, Mark Thompson wrote:
> On 19/01/2020 19:54, Andriy Gelman wrote:
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> > 
> > Hard coded parameters for qmin and qmax are currently used to initialize
> > v4l2_m2m device. This commit uses values from avctx->{qmin,qmax} if they
> > are set.
> > 
> > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> > ---
> >  libavcodec/v4l2_m2m_enc.c | 33 +++++++++++++++++++++++++++++++--
> >  1 file changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
> > index 8059e3bb48f..318be0d3379 100644
> > --- a/libavcodec/v4l2_m2m_enc.c
> > +++ b/libavcodec/v4l2_m2m_enc.c
> > @@ -31,10 +31,25 @@
> >  #include "v4l2_context.h"
> >  #include "v4l2_m2m.h"
> >  #include "v4l2_fmt.h"
> > +#include "internal.h"
> >  
> >  #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
> >  #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
> >  
> > +#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
> > +    do {                                                    \
> > +        if ((in) < (min_val)) {                             \
> > +            av_log((logctx), AV_LOG_WARNING,                \
> > +                   "Adjusted: " name " (%d)\n", (min_val)); \
> > +            in = min_val;                                   \
> > +        }                                                   \
> > +        if ((in) > (max_val)) {                             \
> > +            av_log((logctx), AV_LOG_WARNING,                \
> > +                   "Adjusted: " name " (%d)\n", (max_val)); \
> > +            (in) = (max_val);                               \
> > +        }                                                   \
> > +    } while (0)
> > +
> >  static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
> >  {
> >      struct v4l2_streamparm parm = { 0 };
> > @@ -232,8 +247,15 @@ static int v4l2_prepare_encoder(V4L2m2mContext *s)
> >          return 0;
> >      }
> >  
> > -    if (qmin != avctx->qmin || qmax != avctx->qmax)
> > -        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
> > +    if (avctx->qmin >= 0) {
> > +        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
> > +        qmin = avctx->qmin;
> > +    }
> > +
> > +    if (avctx->qmax >= 0) {
> > +        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
> > +        qmax = avctx->qmax;
> > +    }
> >  
> >      v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
> >      v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
> > @@ -349,6 +371,12 @@ static const AVOption options[] = {
> >      { NULL },
> >  };
> >  
> > +static const AVCodecDefault v4l2_m2m_defaults[] = {
> > +    { "qmin", "-1" },
> > +    { "qmax", "-1" },
> > +    { NULL },
> > +};
> > +
> >  #define M2MENC_CLASS(NAME) \
> >      static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
> >          .class_name = #NAME "_v4l2m2m_encoder", \
> > @@ -370,6 +398,7 @@ static const AVOption options[] = {
> >          .send_frame     = v4l2_send_frame, \
> >          .receive_packet = v4l2_receive_packet, \
> >          .close          = v4l2_encode_close, \
> > +        .defaults       = v4l2_m2m_defaults, \
> >          .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
> >          .wrapper_name   = "v4l2m2m", \
> >      };
> > 

> 
> Can we avoid some of the clumsiness around clipping twice in different ways by querying the quantiser values from the encode device here?
> 

In the s5p-mfc code, a function ends up being assigned to each parameter that
clips the values to be in the correct range. 

As I understand, it's std_validate: 
https://github.com/torvalds/linux/blob/master/drivers/media/v4l2-core/v4l2-ctrls.c#L1873
Then, I do not think we need to explicitly clip the parameter. 

Also, when testing the qmax/qmin, I found it had no affect unless the 
frame_level_rate_control_enable option was set (it's off by default).

If the option is set, qmax/qmin range seems to be respected except for the first
frame, where it's always 1 (not sure why).
diff mbox series

Patch

diff --git a/libavcodec/v4l2_m2m_enc.c b/libavcodec/v4l2_m2m_enc.c
index 8059e3bb48f..318be0d3379 100644
--- a/libavcodec/v4l2_m2m_enc.c
+++ b/libavcodec/v4l2_m2m_enc.c
@@ -31,10 +31,25 @@ 
 #include "v4l2_context.h"
 #include "v4l2_m2m.h"
 #include "v4l2_fmt.h"
+#include "internal.h"
 
 #define MPEG_CID(x) V4L2_CID_MPEG_VIDEO_##x
 #define MPEG_VIDEO(x) V4L2_MPEG_VIDEO_##x
 
+#define CLIP_MIN_MAX(in, min_val, max_val, name, logctx)    \
+    do {                                                    \
+        if ((in) < (min_val)) {                             \
+            av_log((logctx), AV_LOG_WARNING,                \
+                   "Adjusted: " name " (%d)\n", (min_val)); \
+            in = min_val;                                   \
+        }                                                   \
+        if ((in) > (max_val)) {                             \
+            av_log((logctx), AV_LOG_WARNING,                \
+                   "Adjusted: " name " (%d)\n", (max_val)); \
+            (in) = (max_val);                               \
+        }                                                   \
+    } while (0)
+
 static inline void v4l2_set_timeperframe(V4L2m2mContext *s, unsigned int num, unsigned int den)
 {
     struct v4l2_streamparm parm = { 0 };
@@ -232,8 +247,15 @@  static int v4l2_prepare_encoder(V4L2m2mContext *s)
         return 0;
     }
 
-    if (qmin != avctx->qmin || qmax != avctx->qmax)
-        av_log(avctx, AV_LOG_WARNING, "Encoder adjusted: qmin (%d), qmax (%d)\n", qmin, qmax);
+    if (avctx->qmin >= 0) {
+        CLIP_MIN_MAX(avctx->qmin, qmin, qmax, "qmin", avctx);
+        qmin = avctx->qmin;
+    }
+
+    if (avctx->qmax >= 0) {
+        CLIP_MIN_MAX(avctx->qmax, qmin, qmax, "qmax", avctx);
+        qmax = avctx->qmax;
+    }
 
     v4l2_set_ext_ctrl(s, qmin_cid, qmin, "minimum video quantizer scale");
     v4l2_set_ext_ctrl(s, qmax_cid, qmax, "maximum video quantizer scale");
@@ -349,6 +371,12 @@  static const AVOption options[] = {
     { NULL },
 };
 
+static const AVCodecDefault v4l2_m2m_defaults[] = {
+    { "qmin", "-1" },
+    { "qmax", "-1" },
+    { NULL },
+};
+
 #define M2MENC_CLASS(NAME) \
     static const AVClass v4l2_m2m_ ## NAME ## _enc_class = { \
         .class_name = #NAME "_v4l2m2m_encoder", \
@@ -370,6 +398,7 @@  static const AVOption options[] = {
         .send_frame     = v4l2_send_frame, \
         .receive_packet = v4l2_receive_packet, \
         .close          = v4l2_encode_close, \
+        .defaults       = v4l2_m2m_defaults, \
         .capabilities   = AV_CODEC_CAP_HARDWARE | AV_CODEC_CAP_DELAY, \
         .wrapper_name   = "v4l2m2m", \
     };