On Fri, Oct 5, 2018 at 5:38 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > > On 10/05/2018 10:16 AM, Tomasz Figa wrote: > > On Fri, Oct 5, 2018 at 3:58 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: > >> > >> 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? > > > > The latter. > > > >> > >> 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. > > > > 1920x1088 > > > >> > >> 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). > > > > Yes, it is there due to historical reasons. > > > >> > >>> > >>>> > >>>> 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). > > > > We're talking about a decoder here, right? > > > > In this case OUTPUT stream is just a sequence of bytes, not video > > frames, so there is no selection defined for OUTPUT queue. > > > > CAPTURE stream should be seen as a video grabber, so CROP targets > > relate to the encoded rectangle (1920x1088) and COMPOSE targets to the > > CAPTURE buffer. V4L2_SEL_TGT_COMPOSE would be the part of the CAPTURE > > buffer that is written with the image selected by V4L2_SEL_TGT_CROP. > > > > On a decoder that cannot do arbitrary crop and compose, like s5p-mfc, > > both targets would have identical rectangles, equal to the visible > > region (1920x1080). On hardware which can actually do fancier things, > > userspace could freely configure CAPTURE buffer width and height and > > V4L2_SEL_TGT_COMPOSE rectangle, for example to downscale the decoded > > video on the fly. > > > > Please check how I specified all the targets in last version of the > > specification (https://lore.kernel.org/patchwork/patch/966933/) and > > comment there, if there is anything that goes against the > > specification of the selection API. > > I think we all mean the same thing, but just got confused :-) > > > > >> > >> 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. > > Right. > > Please check my RFC series I just posted that hopefully fixes this. > > Specifically https://patchwork.linuxtv.org/patch/52393/ and > https://patchwork.linuxtv.org/patch/52388/ Yeah, I've noticed it, thanks! Added to my list. Best regards, Tomasz
