On 10/05/2018 05:12 AM, Tomasz Figa wrote: > Hi Hans, > > On Fri, Oct 5, 2018 at 5:02 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >> >> Hi all, >> >> I'm looking at removing the last users of vidioc_g/s_crop from the driver and >> I came across vidioc_g_crop in drivers/media/platform/s5p-mfc/s5p_mfc_dec.c. >> >> What this really does AFAICS is return the H.264 frame crop as read from the >> bitstream. This has nothing to do with regular cropping/composing but it might be >> something that could be implemented as a new selection target. > > It has a lot to do, because the output frame buffer may contain (and > on the hardware I worked with, s5p-mfc and mtk-vcodec, indeed does) > the whole encoded stream and the frame crop from the bitstream > specifies the rectangle within it that corresponds to the part that > should be displayed. Yes, but is that part actually cropped? Or is the full uncropped image DMAed to the capture buffer? To take a practical example: a H.264 stream with a 1920x1088 image and a frame crop rectangle of 1920x1080. What is the G_FMT width/height for the decoder capture stream: 1920x1088 or 1920x1080? If it is 1920x1088, then you have a compose rectangle. If it is 1920x1080 then you have a crop rectangle. As far as I can tell from this driver it actually has a compose rectangle and the use of g_crop is wrong and is there due to historical reasons (the driver predates the selection API). > >> >> I'm not really sure what to do with the existing code since it is an abuse of >> the crop API, but I guess the first step is to decide how this should be handled >> properly. >> >> Are there other decoders that can retrieve this information? Should this be >> mentioned in the stateful codec API? > > coda [1], mtk-vcodec [2] and venus [3] expose this using the > V4L2_SEL_TGT_COMPOSE selection target. v1 of the specification defines > the selection targets in a way, which is compatible with that: > V4L2_SEL_TGT_COMPOSE defaults to V4L2_SEL_TGT_COMPOSE_DEFAULT, which > equals to V4L2_SEL_TGT_CROP, which defaults to > V4L2_SEL_TGT_CROP_DEFAULT, which is defined as follows: > > + ``V4L2_SEL_TGT_CROP_DEFAULT`` > + a rectangle covering the part of the frame buffer that contains > + meaningful picture data (visible area); width and height will be > + equal to visible resolution of the stream Where do you get that from? That's the crop definition for an output stream, not a capture stream (assuming we have a codec). I kind of lost you with "which equals to V4L2_SEL_TGT_CROP". In any case, this particular driver should implement g_selection for CAPTURE and implement the COMPOSE targets. That makes sense. Regards, Hans > > AFAIR s5p-mfc was added before the selection API went into active use, > so there is some user space that relies on the crop API. We should > make the driver expose proper selection targets and add a comment next > to the crop API implementation explaining the historical reasons. The > specification should mention only the selection API. > > [1] https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/coda/coda-common.c#L892 > [2] https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c#L740 > [3] https://elixir.bootlin.com/linux/v4.19-rc6/source/drivers/media/platform/qcom/venus/vdec.c#L300 > > Best regards, > Tomasz >
