From: Nicolas Dufresne <nicolas@ndufresne.ca>
To: Hans Verkuil <hverkuil-cisco@xs4all.nl>, linux-media@vger.kernel.org
Cc: Tomasz Figa <tfiga@chromium.org>,
linux-kernel@vger.kernel.org,
Alexandre Courbot <acourbot@chromium.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
Stanimir Varbanov <stanimir.varbanov@linaro.org>,
Andrew-CT Chen <andrew-ct.chen@mediatek.com>,
Tiffany Lin <tiffany.lin@mediatek.com>,
Pawel Osciak <posciak@chromium.org>
Subject: Re: [PATCHv4 0/2] Document memory-to-memory video codec interfaces
Date: Thu, 13 Jun 2019 21:09:35 -0400 [thread overview]
Message-ID: <615f53383f8f65011d1ce3ec49f6d78b67b8ddea.camel@ndufresne.ca> (raw)
In-Reply-To: <259bb812-9cc9-8fe7-8fc6-2cbd5ef44ac3@xs4all.nl>
Le jeudi 13 juin 2019 à 08:48 +0200, Hans Verkuil a écrit :
> 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.
Is the MPEG namespace historical ? That might be confusing for users.
>
> 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.
That's won't be very friendly for UI generator like qv4l2. Support for
v4l2_fract as control should include a way to describe the supported
values of that control the usual way I think.
Also, long term, it would be nice to have two sets of frame rates. The
one that the HW can handle "real-time" and the one that can be used for
bitrate calculation. So staying away from ENUM_FRAMEINTERVALS for
bitrate configuration would be nicer.
> 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 suggested frame rate since this is what I saw implemented by HW
registers (if you think it's worth it, I can try and make a list).
Also, frame-interval steps are not compatible with frame-rate steps
(something that was raised through a venus driver bug) last year. Even
v4l2-ctl was displaying that in a very confusing way. Something as
simple as 1 to 30 fps cannot be exposed through ENU_FRAMEINTERVALS. You
are forced to expose the full fractional range of interval, from 1/30
to 1/1. For Venus it was not that much of a trouble, since its stores a
framerate as Q16..
>
> 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
That's a good point.
> 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
next prev parent reply other threads:[~2019-06-14 1:09 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-03 11:28 [PATCHv4 0/2] Document memory-to-memory video codec interfaces Hans Verkuil
2019-06-03 11:28 ` [PATCHv4 1/2] media: docs-rst: Document memory-to-memory video decoder interface Hans Verkuil
2019-06-10 19:54 ` Nicolas Dufresne
2019-06-11 8:29 ` Hans Verkuil
2019-06-12 0:25 ` Nicolas Dufresne
2019-06-12 6:49 ` Hans Verkuil
2019-06-12 7:02 ` Hans Verkuil
2019-07-15 12:12 ` Maxime Jourdan
2019-07-16 12:23 ` Nicolas Dufresne
2019-07-03 4:58 ` Tomasz Figa
2019-07-10 8:09 ` Hans Verkuil
2019-07-10 8:23 ` Tomasz Figa
2019-07-10 10:00 ` Hans Verkuil
2019-07-17 12:18 ` Nicolas Dufresne
2019-07-19 5:45 ` Tomasz Figa
2019-07-20 3:08 ` Nicolas Dufresne
2019-06-03 11:28 ` [PATCHv4 2/2] media: docs-rst: Document memory-to-memory video encoder interface Hans Verkuil
2019-06-03 14:02 ` [PATCHv4 0/2] Document memory-to-memory video codec interfaces Hans Verkuil
2019-06-04 15:19 ` Nicolas Dufresne
2019-07-03 9:04 ` Tomasz Figa
2019-07-03 17:07 ` Nicolas Dufresne
2019-06-10 15:57 ` Nicolas Dufresne
2019-06-11 8:35 ` Hans Verkuil
2019-06-12 0:33 ` Nicolas Dufresne
2019-06-13 6:48 ` Hans Verkuil
2019-06-14 1:09 ` Nicolas Dufresne [this message]
2019-06-15 8:08 ` Hans Verkuil
2019-06-16 0:17 ` Nicolas Dufresne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=615f53383f8f65011d1ce3ec49f6d78b67b8ddea.camel@ndufresne.ca \
--to=nicolas@ndufresne.ca \
--cc=acourbot@chromium.org \
--cc=andrew-ct.chen@mediatek.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=posciak@chromium.org \
--cc=stanimir.varbanov@linaro.org \
--cc=tfiga@chromium.org \
--cc=tiffany.lin@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).