After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767422 - kmssink: Uses scaled size to specify input buffer region
kmssink: Uses scaled size to specify input buffer region
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.9.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-08 20:25 UTC by Nicolas Dufresne (ndufresne)
Modified: 2016-09-30 07:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
kmssink: Fix selection of source region (2.67 KB, patch)
2016-09-08 15:28 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
kmssink: Scale up to the screen dimension (961 bytes, patch)
2016-09-08 15:28 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2016-06-08 20:25:10 UTC
While testing the new scaling code, I notice it's not doing it right. When I do:

gst-launch-1.0 -v videotestsrc ! video/x-raw,format=NV12,width=640,height=480 ! v4l2video3convert ! video/x-raw,format=NV12_64Z32,width=1920,height=1080 ! kmssink

I get a 1080x1080 image rendered. The image seems scaled up properly, right end of the image is being hidden.
Comment 1 Nicolas Dufresne (ndufresne) 2016-06-10 15:55:49 UTC
Ok, scaling is fine if I force the pixel aspect ratio. This is in fact a bug in kmssink that does not scale to fit the aspect ratio, but yet does not fixate it.
Comment 2 Víctor Manuel Jáquez Leal 2016-06-15 16:29:21 UTC
Hi Nicolas

What do you mean with fixate the aspect ratio?

Does it mean to put in the caps template pixel-aspect-ration=1/1 ???

Somehow I feel it is related with the function gst_video_calculate_device_ratio()
 in gstkmsutil.c
Comment 3 Nicolas Dufresne (ndufresne) 2016-06-15 17:36:57 UTC
If you can get the display PAR (not to confuse with DAR), from the DRM interface, then it should be base on that, otherwise, setting 1/1 is best effort fix yes.
Comment 4 Víctor Manuel Jáquez Leal 2016-07-05 19:30:00 UTC
Nicolas, can you upload a kms*:6 debug log of that?

The thing, if I understand correctly, is display par is calculated here:

https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/kms/gstkmsutils.c#n128

where self->hdisplay, self->vdisplay, self->mm_width and self->mm_height are extracted from DRM
Comment 5 Nicolas Dufresne (ndufresne) 2016-09-08 15:28:09 UTC
Created attachment 335122 [details] [review]
kmssink: Fix selection of source region

The source region was scaled for display before being passed
to drmModeSetPlane, which resulted in a portion of the video
being cropped. While when crop meta was present, the rectangle
was not centered since we where using unscaled width/height.
Comment 6 Nicolas Dufresne (ndufresne) 2016-09-08 15:28:14 UTC
Created attachment 335123 [details] [review]
kmssink: Scale up to the screen dimension

In most display sink, the logic is to use as much as possible
of the given window. In this case, the window is the screen,
hence it's logical to scale up.
Comment 7 Víctor Manuel Jáquez Leal 2016-09-08 15:46:15 UTC
Review of attachment 335122 [details] [review]:

lgtm
Comment 8 Víctor Manuel Jáquez Leal 2016-09-08 15:48:01 UTC
Review of attachment 335123 [details] [review]:

I am not sure about this. IMO users would use this sink as an "overlay", with an UI with a "hole" where the video will be shown. I would keep it as small as possible.
Comment 9 Nicolas Dufresne (ndufresne) 2016-09-08 16:42:05 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #8)
> Review of attachment 335123 [details] [review] [review]:
> 
> I am not sure about this. IMO users would use this sink as an "overlay",
> with an UI with a "hole" where the video will be shown. I would keep it as
> small as possible.

My idea was that it should use as much as possible of the specified rectangle set through gst_video_overlay_set_render_rectangle(). The default rectangle being the entire screen/window. Obviously the next step is to implement this interface. Would it make sense explained this way ?
Comment 10 Víctor Manuel Jáquez Leal 2016-09-08 16:51:10 UTC
(In reply to Nicolas Dufresne (stormer) from comment #9)
> (In reply to Víctor Manuel Jáquez Leal from comment #8)
> > Review of attachment 335123 [details] [review] [review] [review]:
> > 
> > I am not sure about this. IMO users would use this sink as an "overlay",
> > with an UI with a "hole" where the video will be shown. I would keep it as
> > small as possible.
> 
> My idea was that it should use as much as possible of the specified
> rectangle set through gst_video_overlay_set_render_rectangle(). The default
> rectangle being the entire screen/window. Obviously the next step is to
> implement this interface. Would it make sense explained this way ?

Now I got it. It makes sense. Thanks!
Comment 11 Nicolas Dufresne (ndufresne) 2016-09-08 17:40:39 UTC
Attachment 335122 [details] pushed as 37c670e - kmssink: Fix selection of source region
Attachment 335123 [details] pushed as 753c423 - kmssink: Scale up to the screen dimension