GNOME Bugzilla – Bug 727483
v4l2sink: Keep aspect ratio when device scale
Last modified: 2018-11-03 14:52:08 UTC
Created attachment 273456 [details] [review] v4l2sink: Keep aspect ratio when device scale It seems to be the responsibility of the application to ensure that aspect ratio is preserved. The device aspect ratio is constant within a standard. I attached a preliminary work which preserve display aspect ratio of the video. It has been written on 1.2 branch but it should apply on master with a little conflict in v4l2_calls.c Here is the main idea of the solution: First we determine if device can scale using VIDIOC_CROPCAP ioctl by checking if cropcap.bounds and cropcaps.defrect differs. If so we doesn't want to force PAR in caps. Then, when we know video information and if scaling is supported, we determine the target rectangle and we set it with CROP ioctl. This work is not complete since is conflicting with cropping property (crop_*) of v4l2sink element and it scale into output device bounds instead of user-defined display windows. However before working further on this topic, I think it's better to have comments and remarks. Regards
Thanks for looking in to this, I'll have a look and provide feedback as soon as possible.
I've given this a quick look, and it looks pretty neat. I'll definitely consider as soon as I have more time. Thanks for this works.
Created attachment 282138 [details] [review] v4l2: Add a function to probe device cropping capability Since the v4l2 plugins has changed, I updated my previous patch, split it and improve it. Here is the first patch. It adds a function to probe device cropping capabilities. The idea is to get cropcap and try to set a crop window using S_CROP. If underlying device doesn't support crop, it should returns EINVAL. See Exemple 1.11 in http://www.linuxtv.org/downloads/v4l-dvb-apis/crop.html.
Created attachment 282139 [details] [review] v4l2: check if the device can scale This patch intends to check if the device have scaling capabilities. The main idea is to check if crop window can be different from the format. Currently, I only test it with a device which support scaling, I will have a device without scaling support, but I don't know why. So test with a device with no scaler should be done.
Created attachment 282140 [details] [review] v4l2sink: keep aspect ratio when device scale This patch is a proposal to keep display aspect ratio when rendering a video. I think an enhancement could be to add a new property set in v4l2sink, or replace crop-* properties, to define a display window.
Review of attachment 282139 [details] [review]: This patch is not complete because if device supports overlay, I think the crop window should be tested against the overlay window rather than the format. ::: sys/v4l2/v4l2_calls.c @@ +1178,3 @@ v4l2object->vcropcap = cropcap; v4l2object->can_crop = TRUE; + format.type is not set @@ +1200,3 @@ + if (format.fmt.pix_mp.width != crop.c.width || + format.fmt.pix_mp.height != crop.c.height) + v4l2object->can_scale = FALSE; Mistake, should be v4l2object->can_scale = TRUE
Created attachment 285270 [details] [review] v4l2: Add a function to probe device cropping capability Add a debug message and rebase against master
Created attachment 285271 [details] [review] v4l2: check if the device can scale Now we check crop window against overlay windows if device support overlay. It has also been rebased against master and tested with a device with no scaler.
Created attachment 285272 [details] [review] v4l2sink: keep aspect ratio when device scale Rebased against master
Review of attachment 285272 [details] [review]: Just checking, do we keep aspect ratio by default with that path ? ::: sys/v4l2/gstv4l2sink.c @@ +586,3 @@ /* TODO: videosink width/height should be scaled according to * pixel-aspect-ratio */ Can't this comment be removed now ? If not why ?
(In reply to comment #10) > Review of attachment 285272 [details] [review]: > > Just checking, do we keep aspect ratio by default with that path ? Yes unless we can't keep the DAR at all. Which is bad since we have a property to say if we want to keep aspect or not: 'force-aspect-ratio'. :) Maybe, before doing anything, we should check it. > > ::: sys/v4l2/gstv4l2sink.c > @@ +586,3 @@ > /* TODO: videosink width/height should be scaled according to > * pixel-aspect-ratio > */ > > Can't this comment be removed now ? If not why ? At the moment I wrote the patch, I think I didn't know what to do with it. And since i let v4l2sink video_width and video_height, which are unscaled value, i let the comment. But according to it, I should have set the scaled value. By the way, what is the purpose of videosink width and height attributes ?
Hmm, the code after this comment is a no-op: GST_VIDEO_SINK_WIDTH (v4l2sink) = v4l2sink->video_width; GST_VIDEO_SINK_HEIGHT (v4l2sink) = v4l2sink->video_height; Is the same as: v4l2sink->video_width = v4l2sink->video_width; v4l2sink->video_height = v4l2sink->video_height; These though are the width and height of the stream. I must say, this seems bit messy, it's likely the porting was done wrong due to lack of testing.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/112.