Message-ID: <259bb812-9cc9-8fe7-8fc6-2cbd5ef44ac3@xs4all.nl>
Date:   Thu, 13 Jun 2019 08:48:20 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     linux-media@...r.kernel.org
Cc:     Tomasz Figa <tfiga@...omium.org>, linux-kernel@...r.kernel.org,
        Alexandre Courbot <acourbot@...omium.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Tiffany Lin <tiffany.lin@...iatek.com>,
        Pawel Osciak <posciak@...omium.org>
Subject: Re: [PATCHv4 0/2] Document memory-to-memory video codec interfaces

On 6/3/19 1:28 PM, Hans Verkuil wrote:
> Since Tomasz was very busy with other things, I've taken over this
> patch series. This v4 includes his draft changes and additional changes
> from me.
> 
> This series attempts to add the documentation of what was discussed
> during Media Workshops at LinuxCon Europe 2012 in Barcelona and then
> later Embedded Linux Conference Europe 2014 in Düsseldorf and then
> eventually written down by Pawel Osciak and tweaked a bit by Chrome OS
> video team (but mostly in a cosmetic way or making the document more
> precise), during the several years of Chrome OS using the APIs in
> production.
> 
> Note that most, if not all, of the API is already implemented in
> existing mainline drivers, such as s5p-mfc or mtk-vcodec. Intention of
> this series is just to formalize what we already have.
> 
> Thanks everyone for the huge amount of useful comments to previous
> versions of this series. Much of the credits should go to Pawel Osciak
> too, for writing most of the original text of the initial RFC.
> 
> This v4 incorporates all known comments (let me know if I missed
> something!) and should be complete for the decoder.
> 
> For the encoder there are two remaining TODOs for the API:
> 
> 1) Setting the frame rate so bitrate control can make sense, since
>    they need to know this information.
> 
>    Suggested solution: require support for ENUM_FRAMEINTERVALS for the
>    coded pixelformats and S_PARM(OUTPUT). Open question: some drivers
>    (mediatek, hva, coda) require S_PARM(OUTPUT), some (venus) allow both
>    S_PARM(CAPTURE) and S_PARM(OUTPUT). I am inclined to allow both since
>    this is not a CAPTURE vs OUTPUT thing, it is global to both queues.

Alternative proposal:

1) Add support for fractions (struct v4l2_fract) as a control type:
   V4L2_CTRL_TYPE_FRACT.

2) Add a new V4L2_CID_MPEG_FRAME_INTERVAL control.

Encoders shall support this control.

3) For backwards compatibility reasons encoder drivers still have to
support G/S_PARM, but this can now be implemented by standard helpers
that query this control. Drivers also have to implement ENUM_FRAMEINTERVALS.
If the range of intervals is always the same regardless of the frame size,
then a helper can be used that queries the min/max/step of the control, but
if it is dependent on the frame size, then it has to be implemented in the
driver itself.

I'm sticking to frame intervals instead of frame rates for the simple reason
that that's what V4L2 has used since the beginning. I think it is too confusing
to change this to a frame rate. This is just my opinion, though.

I also chose to make this a codec control, not a generic user control: this
value together with the bit rate control(s) determine the compression size,
it does not determine the actual time it takes for the encoder to compress
the raw frames. Hence it is really not the same thing as the frame interval
of a video capture device. If we want to use a control for that as well in
the future as a replacement for G/S_PARM, then that should be a new control.
And we would like need per-pad controls as well in order to implement that.
Which is a lot more work.

Regards,

	Hans

Powered by blists - more mailing lists