| Message ID | 1467322502-11180-1-git-send-email-shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. 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
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. > > 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
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
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c index a01a373..ee7b189 100644 --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c @@ -766,47 +766,6 @@ static const struct v4l2_ctrl_ops s5p_mfc_dec_ctrl_ops = { .g_volatile_ctrl = s5p_mfc_dec_g_v_ctrl, }; -/* Get cropping information */ -static int vidioc_g_crop(struct file *file, void *priv, - struct v4l2_crop *cr) -{ - struct s5p_mfc_ctx *ctx = fh_to_ctx(priv); - struct s5p_mfc_dev *dev = ctx->dev; - u32 left, right, top, bottom; - - if (ctx->state != MFCINST_HEAD_PARSED && - ctx->state != MFCINST_RUNNING && ctx->state != MFCINST_FINISHING - && ctx->state != MFCINST_FINISHED) { - mfc_err("Cannont set crop\n"); - return -EINVAL; - } - if (ctx->src_fmt->fourcc == V4L2_PIX_FMT_H264) { - left = s5p_mfc_hw_call(dev->mfc_ops, get_crop_info_h, ctx); - right = left >> S5P_FIMV_SHARED_CROP_RIGHT_SHIFT; - left = left & S5P_FIMV_SHARED_CROP_LEFT_MASK; - top = s5p_mfc_hw_call(dev->mfc_ops, get_crop_info_v, ctx); - bottom = top >> S5P_FIMV_SHARED_CROP_BOTTOM_SHIFT; - top = top & S5P_FIMV_SHARED_CROP_TOP_MASK; - cr->c.left = left; - cr->c.top = top; - cr->c.width = ctx->img_width - left - right; - cr->c.height = ctx->img_height - top - bottom; - mfc_debug(2, "Cropping info [h264]: l=%d t=%d " - "w=%d h=%d (r=%d b=%d fw=%d fh=%d\n", left, top, - cr->c.width, cr->c.height, right, bottom, - ctx->buf_width, ctx->buf_height); - } else { - cr->c.left = 0; - cr->c.top = 0; - cr->c.width = ctx->img_width; - cr->c.height = ctx->img_height; - mfc_debug(2, "Cropping info: w=%d h=%d fw=%d " - "fh=%d\n", cr->c.width, cr->c.height, ctx->buf_width, - ctx->buf_height); - } - return 0; -} - static int vidioc_decoder_cmd(struct file *file, void *priv, struct v4l2_decoder_cmd *cmd) { @@ -880,7 +839,6 @@ static const struct v4l2_ioctl_ops s5p_mfc_dec_ioctl_ops = { .vidioc_expbuf = vidioc_expbuf, .vidioc_streamon = vidioc_streamon, .vidioc_streamoff = vidioc_streamoff, - .vidioc_g_crop = vidioc_g_crop, .vidioc_decoder_cmd = vidioc_decoder_cmd, .vidioc_subscribe_event = vidioc_subscribe_event, .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
Remove vidioc_g_crop() from s5p-mfc decoder. Without its s_crop counterpart g_crop is not useful. Delete it. Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com> --- drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 42 ---------------------------- 1 file changed, 42 deletions(-)