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

From	Shuah Khan <shuahkh@osg.samsung.com>
Newsgroups	linux.kernel
Subject	Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop
Date	2016-07-05 21:50 +0200
Message-ID	<rREw2-7cF-17@gated-at.bofh.it> (permalink)
References	<rPRQJ-6gv-5@gated-at.bofh.it> <rQMci-73U-17@gated-at.bofh.it> <rQMci-73U-15@gated-at.bofh.it> <rR11D-7HN-1@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On 07/03/2016 07:30 PM, Nicolas Dufresne wrote:
> 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@osg.samsung.com
>>> <mailto:shuahkh@osg.samsung.com>> 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.

Thank you both for the review and comments. I wasn't entirely sure
about removing g-crop, hence this RFC patch. I will add g_selection
and the comment to g_crop about returning incorrect info.

thanks,
-- Shuah

>>
>> 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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[RFC PATCH] media: s5p-mfc - remove vidioc_g_crop Shuah Khan <shuahkh@osg.samsung.com> - 2016-06-30 23:40 +0200
  Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop Hans Verkuil <hverkuil@xs4all.nl> - 2016-07-03 11:50 +0200
    Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop Nicolas Dufresne <nicolas@ndufresne.ca> - 2016-07-04 03:40 +0200
      Re: [RFC PATCH] media: s5p-mfc - remove vidioc_g_crop Shuah Khan <shuahkh@osg.samsung.com> - 2016-07-05 21:50 +0200

csiph-web