Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop

[Nicolas Dufresne]

Le dimanche 03 juillet 2016 Ã 11:43 +0200, Hans Verkuil a ÃcritÂ:
> Hi Nicolas,
> 
> On 07/02/2016 10:29 PM, Nicolas Dufresne wrote:
> > 
> > Le 30 juin 2016 5:35 PM, "Shuah Khan" <shuahkh@xxxxxxxxxxxxxxx
> > <mailto:shuahkh@xxxxxxxxxxxxxxx>> a Ãcrit :
> > > 
> > > Remove vidioc_g_crop() from s5p-mfc decoder. Without its s_crop
> > > counterpart
> > > g_crop is not useful. Delete it.
> > 
> > G_CROP tell the userspace which portion of the output is to be
> > displayed. Example,ÂÂ1920x1080 inside a buffer of 1920x1088. It can
> > be
> > implemented using G_SELECTION too, which emulate G_CROP. removing
> > this without implementing G_SEKECTION will break certain software
> > like
> > GStreamer v4l2 based decoder.
> 
> Sorry, but this is not correct.
> 
> G_CROP for VIDEO_OUTPUT returns the output *compose* rectangle, not
> the output
> crop rectangle.
> 
> Don't blame me, this is how it was defined in V4L2. The problem is
> that for video
> output (esp. m2m devices) you usually want to set the crop rectangle,
> and that's
> why the selection API was added so you can unambiguously set the crop
> and compose
> rectangles for both capture and output.
> 
> Unfortunately, the exynos drivers were written before the
> G/S_SELECTION API was
> created, and the crop ioctls in the video output drivers actually set
> the output
> crop rectangle instead of the compose rectangle :-(
> 
> This is a known inconsistency.
> 
> You are right though that we can't remove g_crop here, I had
> forgotten about the
> buffer padding.
> 
> What should happen here is that g_selection support is added to s5p-
> mfc, and
> have that return the proper rectangles. The g_crop can be kept, and a
> comment
> should be added that it returns the wrong thing, but that that is
> needed for
> backwards compat.
> 
> The gstreamer code should use g/s_selection if available. It should
> check how it
> is using g/s_crop for video output devices today and remember that
> for output
> devices g/s_crop is really g/s_compose, except for the exynos
> drivers.

This is already the case. There is other non-mainline driver that do
like exynos (I have been told).

https://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys?id=7
4f020fd2f1dc645efe35a7ba1f951f9c5ee7c4c

> 
> It's why I recommend the selection API since it doesn't have these
> problems.
> 
> I think I should do another push towards implementing the selection
> API in all
> drivers. There aren't many left.
> 
> Regards,
> 
> 	Hans