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 784599 - kmssink: support videooverlay interface
kmssink: support videooverlay interface
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 765815 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-07-06 04:38 UTC by Haihua Hu
Modified: 2017-12-06 19:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
kmssink: support videooverlay interface (8.99 KB, patch)
2017-07-06 04:38 UTC, Haihua Hu
reviewed Details | Review
kmssink: support videooverlay interface (8.99 KB, patch)
2017-07-06 05:42 UTC, Haihua Hu
none Details | Review
patch refined (9.01 KB, patch)
2017-07-11 07:43 UTC, Haihua Hu
none Details | Review
kmssink: check scaleable when set_caps (1.83 KB, patch)
2017-07-12 02:45 UTC, Haihua Hu
none Details | Review
kmssink video-overlay interface version 3 (8.86 KB, patch)
2017-07-26 11:00 UTC, Haihua Hu
none Details | Review
test app (2.24 KB, text/plain)
2017-07-27 15:26 UTC, Víctor Manuel Jáquez Leal
  Details
kmssink: support videooverlay interface (9.08 KB, patch)
2017-07-27 16:57 UTC, Víctor Manuel Jáquez Leal
none Details | Review
kmssink: support videooverlay interface (version 4) (9.33 KB, patch)
2017-07-31 08:06 UTC, Haihua Hu
none Details | Review
Log for the ENOSPC regressions (7.00 KB, text/x-log)
2017-08-03 15:38 UTC, Nicolas Dufresne (ndufresne)
  Details
kmssink: add can-scale property (2.41 KB, patch)
2017-09-12 10:52 UTC, Haihua Hu
committed Details | Review
kmssink: support videooverlay interface (version 5) (10.23 KB, patch)
2017-09-12 10:53 UTC, Haihua Hu
none Details | Review
kmssink: support videooverlay interface (version 6) (10.29 KB, patch)
2017-09-13 02:20 UTC, Haihua Hu
needs-work Details | Review
kms overlay test app (v2) (2.58 KB, text/plain)
2017-09-22 10:42 UTC, Víctor Manuel Jáquez Leal
  Details
kmssink: support videooverlay interface (version 7) (10.60 KB, patch)
2017-11-15 06:31 UTC, Wonchul Lee
none Details | Review
kmssink: support videooverlay interface (version 7) (10.54 KB, patch)
2017-12-01 06:48 UTC, Wonchul Lee
committed Details | Review
kmssink: Add display-width/height properties (3.84 KB, patch)
2017-12-01 17:02 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
Updated test app with display-width/height support (3.43 KB, text/x-csrc)
2017-12-01 17:03 UTC, Nicolas Dufresne (ndufresne)
  Details
kmssink: Enforce pixel aspect ratio when we cannot scale (6.16 KB, patch)
2017-12-01 21:56 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Haihua Hu 2017-07-06 04:38:02 UTC
implement videooverlay interface in kmssink, divided into two case when driver support scale, then we do refresh in show_frame, if not, send a reconfigure event upstream and do re-negotiate using the new size.
Comment 1 Haihua Hu 2017-07-06 04:38:27 UTC
Created attachment 354995 [details] [review]
kmssink: support videooverlay interface
Comment 2 Haihua Hu 2017-07-06 05:42:56 UTC
Created attachment 355004 [details] [review]
kmssink: support videooverlay interface
Comment 3 Nicolas Dufresne (ndufresne) 2017-07-06 13:26:51 UTC
Review of attachment 354995 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +724,3 @@
+        "height", G_TYPE_INT, self->preferred_height, NULL);
+  }
+  GST_DEBUG_OBJECT (self, "allowed caps %"GST_PTR_FORMAT, caps);

I don't think overriding the width/height is correct here. Instead, you should prepand you preferred caps, and keep the other valid caps. And you should do that always.

@@ +732,3 @@
     out_caps = caps;
   }
+  g_mutex_unlock (&self->glock);

You seem to be holding the lock a bit too long, just a nitpick.

@@ +873,3 @@
 
+  if (self->reconfigure) {
+    self->reconfigure = FALSE;

Need the glock ?

@@ +1515,3 @@
   sink->plane_id = -1;
+  sink->reconfigure = FALSE;
+  g_mutex_init (&sink->glock);

Are you sure the GST_OBJECT_LOCK() could not have made the job here ? It's only used for math, it never does any calls around.
Comment 4 Nicolas Dufresne (ndufresne) 2017-07-06 13:27:45 UTC
Comment on attachment 355004 [details] [review]
kmssink: support videooverlay interface

Same comments apply I believe.
Comment 5 Víctor Manuel Jáquez Leal 2017-07-06 13:36:06 UTC
*** Bug 765815 has been marked as a duplicate of this bug. ***
Comment 6 Víctor Manuel Jáquez Leal 2017-07-06 13:43:44 UTC
Review of attachment 355004 [details] [review]:

I guess you missed to pass the code through gst-indent

::: sys/kms/gstkmssink.c
@@ +574,3 @@
 
+  self->hdisplay = self->preferred_width = crtc->mode.hdisplay;
+  self->vdisplay = self->preferred_height= crtc->mode.vdisplay;

code style (space between = )

@@ +724,3 @@
+        "height", G_TYPE_INT, self->preferred_height, NULL);
+  }
+  GST_DEBUG_OBJECT (self, "allowed caps %"GST_PTR_FORMAT, caps);

ditto

@@ +733,3 @@
   }
+  g_mutex_unlock (&self->glock);
+  GST_DEBUG_OBJECT (self, "out caps %"GST_PTR_FORMAT, out_caps);

ditto

@@ +1261,3 @@
+      dst.h = height;
+
+      gst_video_sink_center_rect (src, dst, &result, TRUE );

ditto

@@ +1264,3 @@
+
+      kmssink->preferred_x = x+result.x;
+      kmssink->preferred_y = y+result.y;

ditto
Comment 7 Víctor Manuel Jáquez Leal 2017-07-06 13:50:47 UTC
Review of attachment 355004 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +1333,3 @@
+    self->original_heigth = GST_VIDEO_INFO_HEIGHT (&self->vinfo);
+  }
+

wouldn't be clearer to set this in gst_kms_sink_set_caps() when reconfigure is TRUE?

::: sys/kms/gstkmssink.h
@@ +77,3 @@
+  gint preferred_y;
+  gint preferred_width;
+  gint preferred_height;

wouldn't be better to use a GstVideoRectangle here instead of these four integers?
Comment 8 Víctor Manuel Jáquez Leal 2017-07-06 13:51:32 UTC
Also I would try to factor out the math to keep the lock logic isolated.
Comment 9 Haihua Hu 2017-07-11 02:56:26 UTC
Thanks for your comments, I am refining my patch and will submit today.
Comment 10 Haihua Hu 2017-07-11 07:43:57 UTC
Created attachment 355313 [details] [review]
patch refined
Comment 11 Nicolas Dufresne (ndufresne) 2017-07-11 11:40:40 UTC
Review of attachment 355313 [details] [review]:

Just a note, set window handle is not implement, what happens if we call this ?
Comment 12 Haihua Hu 2017-07-12 01:30:23 UTC
(In reply to Nicolas Dufresne (stormer) from comment #11)
> Review of attachment 355313 [details] [review] [review]:
> 
> Just a note, set window handle is not implement, what happens if we call
> this ?

DRM can work without window system. As below code show that if we call set window handle, then noting will happen. User should do window integrate in application. e.g, resize the window and get the window size and position and set them to kmssink.

335 void                                                                                                                                                                                 
336 gst_video_overlay_set_window_handle (GstVideoOverlay * overlay, guintptr handle)                                                                                                     
337 {                                                                                                                                                                                    
338   GstVideoOverlayInterface *iface;                                                                                                                                                   
339                                                                                                                                                                                      
340   g_return_if_fail (overlay != NULL);                                                                                                                                                
341   g_return_if_fail (GST_IS_VIDEO_OVERLAY (overlay));                                                                                                                                 
342                                                                                                                                                                                      
343   iface = GST_VIDEO_OVERLAY_GET_INTERFACE (overlay);                                                                                                                                 
344                                                                                                                                                                                      
345   if (iface->set_window_handle) {                                                                                                                                                    
346     iface->set_window_handle (overlay, handle);                                                                                                                                      
347   }                                                                                                                                                                                  
348 }
Comment 13 Haihua Hu 2017-07-12 02:45:14 UTC
Created attachment 355380 [details] [review]
kmssink: check scaleable when set_caps

There is a bug when driver not support scale. when video size is same as display resolution, show frame will not do retry because the setplane will never fail. But the scale result is not correct. We can do this check by set a fake plane.

This bug will impact the videooverlay interface, because the implementation is based on driver scaleable capability.
Comment 14 Haihua Hu 2017-07-18 02:04:55 UTC
Hi all,

Any updates?
Comment 15 Víctor Manuel Jáquez Leal 2017-07-18 12:09:39 UTC
Review of attachment 355313 [details] [review]:

Sorry, but first you need to rebase the patch :(
Comment 16 Víctor Manuel Jáquez Leal 2017-07-18 12:18:39 UTC
Review of attachment 355313 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +728,3 @@
+  GST_OBJECT_UNLOCK (self);
+
+  GST_DEBUG_OBJECT (self, "allowed caps %" GST_PTR_FORMAT, caps);

this debug message can be removed. I would remove both (the next added one too) because they can be known with other logs, but have one is OK.

@@ +1250,3 @@
 
+static void
+gst_kms_sink_set_render_rectangle (GstVideoOverlay * overlay,

please move these three function up, as the first functions of the file, so we could identify quickly as the block related with the overlay interface.

@@ +1253,3 @@
+    gint x, gint y, gint width, gint height)
+{
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);

please use 'self' instead of 'kmssink' to keep the consistence among the code.

@@ +1255,3 @@
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);
+
+  if (width > 0 && height > 0) {

I would use here a quick return idiomatic:

if (width == 0 || height == 0)
  return

thus we can save some indentation ;)

@@ +1291,3 @@
+gst_kms_sink_expose (GstVideoOverlay * overlay)
+{
+  GstKMSSink *kmssink = GST_KMS_SINK (overlay);

self instead of kmssink

@@ +1336,3 @@
   if (!buffer)
     return GST_FLOW_ERROR;
+

I guess there's no reason to add this space.

@@ +1343,3 @@
   GST_TRACE_OBJECT (self, "displaying fb %d", fb_id);
 
+  GST_OBJECT_LOCK (self);

I'm a bit worried about this long lock. Is it truly required? Just wondering...

@@ +1384,3 @@
+  src.h =
+      (result.y + result.h) >
+      self->vdisplay ? self->vdisplay - result.y : src.h;

these conditional operators are hard to read when they are not one-liners. I would change them to normal if ()
Comment 17 Víctor Manuel Jáquez Leal 2017-07-18 12:20:44 UTC
Review of attachment 355380 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +358,3 @@
+  if (result) {
+    self->can_scale = FALSE;
+    GST_INFO_OBJECT (self, "scale is not support");

I don't like the idea of showing an empty (green?) frame to the user at start. Can we delay the expose call until the first show_frame() is called.
Comment 18 Víctor Manuel Jáquez Leal 2017-07-19 10:46:49 UTC
Review of attachment 355313 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +1294,3 @@
+
+  if (kmssink->can_scale) {
+    gst_kms_sink_show_frame (kmssink, NULL);

you need to cast here to GST_VIDEO_SINK
Comment 19 Haihua Hu 2017-07-19 10:59:29 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #17)
> Review of attachment 355380 [details] [review] [review]:
> 

Thanks for your comments.

> ::: sys/kms/gstkmssink.c
> @@ +358,3 @@
> +  if (result) {
> +    self->can_scale = FALSE;
> +    GST_INFO_OBJECT (self, "scale is not support");
> 
> I don't like the idea of showing an empty (green?) frame to the user at
> start. Can we delay the expose call until the first show_frame() is called.

Is there any other way to check whether driver support scale or not? Why delay expose can fix this problem?
Comment 20 Víctor Manuel Jáquez Leal 2017-07-19 11:09:25 UTC
(In reply to Haihua Hu from comment #19)
> (In reply to Víctor Manuel Jáquez Leal from comment #17)
> > Review of attachment 355380 [details] [review] [review] [review]:
> > 
> 
> Thanks for your comments.
> 
> > ::: sys/kms/gstkmssink.c
> > @@ +358,3 @@
> > +  if (result) {
> > +    self->can_scale = FALSE;
> > +    GST_INFO_OBJECT (self, "scale is not support");
> > 
> > I don't like the idea of showing an empty (green?) frame to the user at
> > start. Can we delay the expose call until the first show_frame() is called.
> 
> Is there any other way to check whether driver support scale or not? Why
> delay expose can fix this problem?

As far as I understand, the element will know if the driver supports scale after the first _show_frame() call.

I guess, somehow _expose() will call first _show_frame() and then it will know if the driver can scale.
Comment 21 Haihua Hu 2017-07-26 11:00:32 UTC
Created attachment 356413 [details] [review]
kmssink video-overlay interface version 3
Comment 22 Haihua Hu 2017-07-26 11:04:59 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #20)
> (In reply to Haihua Hu from comment #19)
> > (In reply to Víctor Manuel Jáquez Leal from comment #17)
> > > Review of attachment 355380 [details] [review] [review] [review] [review]:
> > > 
> > 
> > Thanks for your comments.
> > 
> > > ::: sys/kms/gstkmssink.c
> > > @@ +358,3 @@
> > > +  if (result) {
> > > +    self->can_scale = FALSE;
> > > +    GST_INFO_OBJECT (self, "scale is not support");
> > > 
> > > I don't like the idea of showing an empty (green?) frame to the user at
> > > start. Can we delay the expose call until the first show_frame() is called.
> > 
> > Is there any other way to check whether driver support scale or not? Why
> > delay expose can fix this problem?
> 
> As far as I understand, the element will know if the driver supports scale
> after the first _show_frame() call.

Hi, this is not correct. If the original video size is equal to display size. The first setplane try will not fail. As a result, the can_scale will be true even if the drm driver don't support scale. We should make sure the can_scale be correct when start.

> 
> I guess, somehow _expose() will call first _show_frame() and then it will
> know if the driver can scale.
Comment 23 Víctor Manuel Jáquez Leal 2017-07-26 16:25:19 UTC
Review of attachment 356413 [details] [review]:

looks good to me.

Any comment @Nicolas?
Comment 24 Nicolas Dufresne (ndufresne) 2017-07-26 18:09:48 UTC
Review of attachment 356413 [details] [review]:

Looks good indeed, I only have one question below about locking. Is the other patch obsolete now ?

::: sys/kms/gstkmssink.c
@@ +1293,3 @@
+
+  if (self->can_scale) {
+    gst_kms_sink_show_frame (GST_VIDEO_SINK (self), NULL);

Are you missing some locking here ? Normally show_frame() is called with stream lock held, but now it's called from application thread, no locks. Have you verified/protected everything inside that function ?
Comment 25 Haihua Hu 2017-07-27 02:40:09 UTC
(In reply to Nicolas Dufresne (stormer) from comment #24)
> Review of attachment 356413 [details] [review] [review]:
> 
> Looks good indeed, I only have one question below about locking. Is the
> other patch obsolete now ?

Old videooverlay interface patch has been obsolete. Then how about the check scaleable patch, if not ok, then is there other better way for me to check this feature? 

> 
> ::: sys/kms/gstkmssink.c
> @@ +1293,3 @@
> +
> +  if (self->can_scale) {
> +    gst_kms_sink_show_frame (GST_VIDEO_SINK (self), NULL);
> 
> Are you missing some locking here ? Normally show_frame() is called with
> stream lock held, but now it's called from application thread, no locks.
> Have you verified/protected everything inside that function ?

We have lock in show_frame function GST_OBJECT_LOCK (self).
Comment 26 Nicolas Dufresne (ndufresne) 2017-07-27 12:00:04 UTC
Was Victor's comment solved? Having to render a null frame is taking us away from being able to do seamless transition, like from kmssink to Wayland.
Comment 27 Haihua Hu 2017-07-27 12:06:53 UTC
(In reply to Nicolas Dufresne (stormer) from comment #26)
> Was Victor's comment solved? Having to render a null frame is taking us away
> from being able to do seamless transition, like from kmssink to Wayland.

Sorry, which comment?
Comment 28 Nicolas Dufresne (ndufresne) 2017-07-27 12:37:50 UTC
Comment 17 (https://bugzilla.gnome.org/show_bug.cgi?id=784599#c17). You then asked us if there is another way to check for scaling, I'd aimed toward DRM mailing list for that.
Comment 29 Haihua Hu 2017-07-27 12:44:46 UTC
(In reply to Nicolas Dufresne (stormer) from comment #28)
> Comment 17 (https://bugzilla.gnome.org/show_bug.cgi?id=784599#c17). You then
> asked us if there is another way to check for scaling, I'd aimed toward DRM
> mailing list for that.

Thanks, we can open another thread to track the scaling issue. That will be great if you got something useful from DRM Mail list.
Comment 30 Víctor Manuel Jáquez Leal 2017-07-27 14:02:54 UTC
Sorry, the code needs to be rebased again. I'm doing it right now. Also I'm moving some code around just to make it cleaner, and I will push it.

I'm talking about attachment 356413 [details] [review] only, for now. I don't feel comfortable with the other patch yet. Still, I doing some tests.
Comment 31 Víctor Manuel Jáquez Leal 2017-07-27 15:26:14 UTC
Created attachment 356481 [details]
test app

This a simple test app that moves the output of videotestsrc in a diagonal across the screen.
Comment 32 Víctor Manuel Jáquez Leal 2017-07-27 15:37:57 UTC
Review of attachment 356413 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +1254,3 @@
+
+  if (width == 0 || height == 0)
+    return;

We might have a problem here. According to the documentation [1], having -1 in width and height is valid:

"To unset the region pass -1 for the width and height parameters."

It's not clear to me what it means in our case, shall we reset the preferrect_rect? 

For me, it would be more natural, if w/h are -1, use the original w/h

1. https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/GstVideoOverlay.html#gst-video-overlay-set-render-rectangle

@@ +1384,3 @@
+
+  if (src.w <= 0 || src.h <= 0) {
+    GST_DEBUG_OBJECT (self, "video totally out of range");

this should be a GST_WARNING_OBJECT(), without the "totally" adjective ;)

@@ +1524,3 @@
   sink->conn_id = -1;
   sink->plane_id = -1;
+  sink->reconfigure = FALSE;

this is not required, since the instance structure is zero-ed (less code is good)
Comment 33 Haihua Hu 2017-07-27 16:20:51 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #32)
> Review of attachment 356413 [details] [review] [review]:
> 
> ::: sys/kms/gstkmssink.c
> @@ +1254,3 @@
> +
> +  if (width == 0 || height == 0)
> +    return;

I'm also confused about this, in glimagesink, if (x < 0 || y < 0 || width <= 0 || height <= 0) it will return also. don't do anthing. Then how to unset this region, do we need record the previous size?

Hi Nicolas,
what's your opinion?

> 
> We might have a problem here. According to the documentation [1], having -1
> in width and height is valid:
> 
> "To unset the region pass -1 for the width and height parameters."
> 
> It's not clear to me what it means in our case, shall we reset the
> preferrect_rect? 
> 
> For me, it would be more natural, if w/h are -1, use the original w/h
> 
> 1.
> https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-
> libs/html/GstVideoOverlay.html#gst-video-overlay-set-render-rectangle
> 
> @@ +1384,3 @@
> +
> +  if (src.w <= 0 || src.h <= 0) {
> +    GST_DEBUG_OBJECT (self, "video totally out of range");
> 
> this should be a GST_WARNING_OBJECT(), without the "totally" adjective ;)
> 
> @@ +1524,3 @@
>    sink->conn_id = -1;
>    sink->plane_id = -1;
> +  sink->reconfigure = FALSE;
> 
> this is not required, since the instance structure is zero-ed (less code is
> good)

Do I need update the patch? :) or you are doing this? just avoid duplicate working.
Comment 34 Víctor Manuel Jáquez Leal 2017-07-27 16:30:50 UTC
(In reply to Haihua Hu from comment #33)
> Do I need update the patch? :) or you are doing this? just avoid duplicate
> working.

Let me repost the patch with my "small" changes. Still the -1 size is still pending to do.
Comment 35 Víctor Manuel Jáquez Leal 2017-07-27 16:57:19 UTC
Created attachment 356483 [details] [review]
kmssink: support videooverlay interface

Implement videooverlay interface in kmssink, divided into two cases:
when driver supports scale, then we do refresh in show_frame(); if
not, send a reconfigure event to upstream and re-negotiate, using the
new size.

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Comment 36 Víctor Manuel Jáquez Leal 2017-07-27 16:58:52 UTC
Review of attachment 356413 [details] [review]:

mark this as reject to continue reviewing the next version of the patch
Comment 37 Nicolas Dufresne (ndufresne) 2017-07-27 17:40:40 UTC
glimagesink has a bug then. -1/-1 should indeed unset the region and revert back to covering as much as possible the given surface. For kmssink, this is reverting back to the default behaviour.
Comment 38 Haihua Hu 2017-07-28 01:47:56 UTC
Review of attachment 356483 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +97,3 @@
+  GstKMSSink *self = GST_KMS_SINK (overlay);
+
+  if (width == 0 || height == 0)

We need add FIXME here if it should be fix later, and I think below is better

if (width <= 0 || height <= 0)
   return;
Comment 39 Víctor Manuel Jáquez Leal 2017-07-28 17:21:49 UTC
(In reply to Haihua Hu from comment #38)
> Review of attachment 356483 [details] [review] [review]:
> 
> ::: sys/kms/gstkmssink.c
> @@ +97,3 @@
> +  GstKMSSink *self = GST_KMS_SINK (overlay);
> +
> +  if (width == 0 || height == 0)
> 
> We need add FIXME here if it should be fix later, and I think below is better

There is no reason to merge an incomplete feature. It doesn't seem too complex to get it done. Can you update the patch?
Comment 40 Haihua Hu 2017-07-31 08:06:08 UTC
Created attachment 356624 [details] [review]
kmssink: support videooverlay interface (version 4)
Comment 41 Haihua Hu 2017-07-31 08:06:54 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #39)
> (In reply to Haihua Hu from comment #38)
> > Review of attachment 356483 [details] [review] [review] [review]:
> > 
> > ::: sys/kms/gstkmssink.c
> > @@ +97,3 @@
> > +  GstKMSSink *self = GST_KMS_SINK (overlay);
> > +
> > +  if (width == 0 || height == 0)
> > 
> > We need add FIXME here if it should be fix later, and I think below is better
> 
> There is no reason to merge an incomplete feature. It doesn't seem too
> complex to get it done. Can you update the patch?

Sorry for late, I have update the patch.
Comment 42 Víctor Manuel Jáquez Leal 2017-07-31 18:49:06 UTC
Attachment 356483 [details] pushed as db07f45 - kmssink: support videooverlay interface
Comment 43 Víctor Manuel Jáquez Leal 2017-07-31 18:51:49 UTC
I have updated a bit the patch for readability.

Regarding attach 355380, I don't know what do you prefer: close this bug and open a new one; or keep working on this bug report.
Comment 44 Haihua Hu 2017-08-01 01:27:33 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #43)
> I have updated a bit the patch for readability.
> 
> Regarding attach 355380, I don't know what do you prefer: close this bug and
> open a new one; or keep working on this bug report.

close this bug and open a new one. I will create one.

Hi Nicolas,
Have you got some message from DRM mail list for scale issue?
Comment 45 Nicolas Dufresne (ndufresne) 2017-08-01 01:48:06 UTC
Oh, sorry, I'm not watching this ML these days. I meant that someone should ask.
Comment 46 Haihua Hu 2017-08-01 03:10:13 UTC
Hi All,
let's close this bug and continue tracking the scale issue in this thread: 
https://bugzilla.gnome.org/show_bug.cgi?id=785663
Comment 47 Nicolas Dufresne (ndufresne) 2017-08-01 20:55:43 UTC
This patch causes regression on MSM DRM (DB410c).

We get ENOSPC, kernel error:
  Invalid source coordinates 320.000000x264.000000+0.000000+0.000000
Comment 48 Haihua Hu 2017-08-02 01:29:35 UTC
(In reply to Nicolas Dufresne (stormer) from comment #47)
> This patch causes regression on MSM DRM (DB410c).
> 
> We get ENOSPC, kernel error:
>   Invalid source coordinates 320.000000x264.000000+0.000000+0.000000

I have no msm board, can not have a test, does it support scale?
Comment 49 Nicolas Dufresne (ndufresne) 2017-08-02 16:06:58 UTC
Yes, it supports scaling. But it fails a generic condition in DRM framework when the input is 320x240, so clearly something is broken in general in this patch, see drm_atomic_plane_check/drm_framebuffer_check_src_coords. The FB is probably set to 240, but this code will use 264.
Comment 50 Haihua Hu 2017-08-03 01:18:23 UTC
(In reply to Nicolas Dufresne (stormer) from comment #49)
> Yes, it supports scaling. But it fails a generic condition in DRM framework
> when the input is 320x240, so clearly something is broken in general in this
> patch, see drm_atomic_plane_check/drm_framebuffer_check_src_coords. The FB
> is probably set to 240, but this code will use 264.

can you provide some log from kmssink?
Comment 51 Nicolas Dufresne (ndufresne) 2017-08-03 15:38:37 UTC
Created attachment 356870 [details]
Log for the ENOSPC regressions
Comment 52 Nicolas Dufresne (ndufresne) 2017-08-03 15:39:59 UTC
Notice, it says "scaling to 320x264", which seems wrong.
Comment 53 Nicolas Dufresne (ndufresne) 2017-08-03 15:45:44 UTC
Actually, the most suspicious (and the delta with before is "sourcing at (0,0) 320x264" instead of "sourcing at (0,0) 320x240). All those traces looks odd, they seems slightly counter intuitive, so maybe they hide a bug, or maybe they are just not what I think they are.
Comment 54 Nicolas Dufresne (ndufresne) 2017-08-07 18:11:48 UTC
Btw, this patch fails if you have non-squared pixels, which is my case when running in 1920x1200 (apparently). The problem is that you confused display vs source. You should never scale the src rectangle, whatever happens, if you have 320x240, your largest source rectangle will be 0,0 320x240. Right now, this patch forgets the actual size provided in the caps, it's quite unfortunate.
Comment 55 Nicolas Dufresne (ndufresne) 2017-08-07 18:20:39 UTC
And this hacky original_width/height, this seems bogus, it's never reset. Did you check what happens if the input resolution changes at run-time ?
Comment 56 Nicolas Dufresne (ndufresne) 2017-08-07 20:48:56 UTC
Review of attachment 356624 [details] [review]:

I tried to fix it myself, but it's not exactly trivial. I have reverted it, so we gain time to fix it properly.

::: sys/kms/gstkmssink.c
@@ -1327,3 @@
-  } else {
-    src.w = GST_VIDEO_INFO_WIDTH (&self->vinfo);
-    src.h = GST_VIDEO_INFO_HEIGHT (&self->vinfo);

Removing this code is what caused the regression. The idea in this blob is to replace src.w/h scaled value with non-scaled one. With this patch, you don't do that anymore, as a result, you give a scaled value to drmModeSetPlane which can lead to ENOSPC.

Now, the old code is assuming that if the driver cannot scale, you don't need to clip the src rectangle (that DRM will clip for us. I would need to refer to the doc on what shall be expect from drivers. I should probably hack the can_scale locally to be able to test that.

@@ +1447,3 @@
+
+  if ((result.y + result.h) > self->vdisplay)
+    src.h = self->vdisplay - result.y;

Here that code seem wrong, you check if result (the destination rectangle) overflow the display, but clip the src rectangle.

@@ +1457,3 @@
+  if (!self->can_scale) {
+    result.w = src.w;
+    result.h = src.h;

What if the src.w/h is larder then your display ?

@@ +1631,3 @@
   sink->plane_id = -1;
+  sink->original_width = -1;
+  sink->original_heigth = -1;

This need to be reset on _start() I believe. Though, I'm not even sure we need this.
Comment 57 Haihua Hu 2017-08-08 02:18:25 UTC
(In reply to Nicolas Dufresne (stormer) from comment #53)
> Actually, the most suspicious (and the delta with before is "sourcing at
> (0,0) 320x264" instead of "sourcing at (0,0) 320x240). All those traces
> looks odd, they seems slightly counter intuitive, so maybe they hide a bug,
> or maybe they are just not what I think they are.

Hi Nicolas,

Sorry for late reply to you, the root cause located in gst_kms_sink_calculate_display_ratio, the result of this calculation on your side is 320*264, see below log:

0:00:08.763885491  3486     0x2c77f4a0 DEBUG                kmssink gstkmssink.c:937:gst_kms_sink_calculate_display_ratio:<kmssink0> video calculated display ratio: 40/33
0:00:08.763933043  3486     0x2c77f4a0 DEBUG                kmssink gstkmssink.c:951:gst_kms_sink_calculate_display_ratio:<kmssink0> keeping video width
0:00:08.763964814  3486     0x2c77f4a0 DEBUG                kmssink gstkmssink.c:962:gst_kms_sink_calculate_display_ratio:<kmssink0> scaling to 320x264

I think this calculation is not correct. It will respect to this actually display width and height in millimeter but not pixel size. I don't know why here should do like this.
Comment 58 Haihua Hu 2017-08-08 09:28:35 UTC
(In reply to Nicolas Dufresne (stormer) from comment #56)
> Review of attachment 356624 [details] [review] [review]:
> 
> I tried to fix it myself, but it's not exactly trivial. I have reverted it,
> so we gain time to fix it properly.
> 
> ::: sys/kms/gstkmssink.c
> @@ -1327,3 @@
> -  } else {
> -    src.w = GST_VIDEO_INFO_WIDTH (&self->vinfo);
> -    src.h = GST_VIDEO_INFO_HEIGHT (&self->vinfo);
> 
> Removing this code is what caused the regression. The idea in this blob is
> to replace src.w/h scaled value with non-scaled one. With this patch, you
> don't do that anymore, as a result, you give a scaled value to
> drmModeSetPlane which can lead to ENOSPC.
> 
> Now, the old code is assuming that if the driver cannot scale, you don't
> need to clip the src rectangle (that DRM will clip for us. I would need to
> refer to the doc on what shall be expect from drivers. I should probably
> hack the can_scale locally to be able to test that.

Yes, it is the reason why this issue happen, when display is not a regular screen, the GST_VIDEO_INFO_WIDTH and GST_VIDEO_INFO_HEIGHT will be different with the video size in video info, caps. So this two value should never use for fb size.
> 
> @@ +1447,3 @@
> +
> +  if ((result.y + result.h) > self->vdisplay)
> +    src.h = self->vdisplay - result.y;
> 
> Here that code seem wrong, you check if result (the destination rectangle)
> overflow the display, but clip the src rectangle.

we should clip the video can not been displayed, other wise driver will return error

> 
> @@ +1457,3 @@
> +  if (!self->can_scale) {
> +    result.w = src.w;
> +    result.h = src.h;
> 
> What if the src.w/h is larder then your display ?
> 
> @@ +1631,3 @@
>    sink->plane_id = -1;
> +  sink->original_width = -1;
> +  sink->original_heigth = -1;
> 
> This need to be reset on _start() I believe. Though, I'm not even sure we
> need this.

We need this info, when driver not support scale, then we need send the new size upstream to find some sw scale do this, we prefer to keep aspect ratio then we need know the original video size. I will reset it in _start()

I will try to fix those issues and re-upload my patch
Comment 59 Nicolas Dufresne (ndufresne) 2017-08-08 14:58:56 UTC
(In reply to Haihua Hu from comment #57)
> 0:00:08.763964814  3486     0x2c77f4a0 DEBUG                kmssink
> gstkmssink.c:962:gst_kms_sink_calculate_display_ratio:<kmssink0> scaling to
> 320x264
> 
> I think this calculation is not correct. It will respect to this actually
> display width and height in millimeter but not pixel size. I don't know why
> here should do like this.

The trace is missleading, but the calculation is correct. It's the right value you should pass to gst_video_sink_center_rect() when scaling to compensate the different pixel ratio between video pixels and display pixels.

When the driver does not scale, then it cannot do DAR compensation. As we don't know that it's scaling in advance, we cannot do much about it. We endup having to render the buffer incorrectly. This would be done by ignoring DAR compensation. So we would just skip gst_kms_sink_calculate_display_ratio() when can_scale is FALSE and hardcode SINK_WIDTH/HEIGHT to vinfo.width/height.

Then, when you renegotiate, and you have can_scale = FALSE, you should fixate the pixel-ascpect-ratio to the display pixel aspect ratio. This way, upstream scaler can take case.

The original_width/height is to my opnion useless. What you want instead, is create a caps with <preferred>/<range>. The preferred width/height is the display size (which you already have).
Comment 60 Haihua Hu 2017-08-10 05:28:35 UTC
(In reply to Nicolas Dufresne (stormer) from comment #59)
> (In reply to Haihua Hu from comment #57)
> > 0:00:08.763964814  3486     0x2c77f4a0 DEBUG                kmssink
> > gstkmssink.c:962:gst_kms_sink_calculate_display_ratio:<kmssink0> scaling to
> > 320x264
> > 
> > I think this calculation is not correct. It will respect to this actually
> > display width and height in millimeter but not pixel size. I don't know why
> > here should do like this.
> 
> The trace is missleading, but the calculation is correct. It's the right
> value you should pass to gst_video_sink_center_rect() when scaling to
> compensate the different pixel ratio between video pixels and display pixels.
> 
> When the driver does not scale, then it cannot do DAR compensation. As we
> don't know that it's scaling in advance, we cannot do much about it. We
> endup having to render the buffer incorrectly. This would be done by
> ignoring DAR compensation. So we would just skip
> gst_kms_sink_calculate_display_ratio() when can_scale is FALSE and hardcode
> SINK_WIDTH/HEIGHT to vinfo.width/height.

This is an good ideal,but the problem is that we don't know for sure whether the driver support scale until we try to show first frame. How to solve this problem?

> 
> Then, when you renegotiate, and you have can_scale = FALSE, you should
> fixate the pixel-ascpect-ratio to the display pixel aspect ratio. This way,
> upstream scaler can take case.
> 
> The original_width/height is to my opnion useless. What you want instead, is
> create a caps with <preferred>/<range>. The preferred width/height is the
> display size (which you already have).
Comment 61 Nicolas Dufresne (ndufresne) 2017-08-10 12:29:49 UTC
For now, all we can do is to center pixel wise, and hope for a renegotion.

The real solution, is to port to atomic DRM. The new API allow trying a configuration without actually doing a render. This way we can detect in set_format if we can scale or not, and negotiate accordingly.

On TI/Sitara branch, I notice they added a property to indicate to scale or not. That could also be a shorter term solution if Victor agree.
Comment 62 Víctor Manuel Jáquez Leal 2017-08-10 14:20:47 UTC
(In reply to Nicolas Dufresne (stormer) from comment #61)

> On TI/Sitara branch, I notice they added a property to indicate to scale or
> not. That could also be a shorter term solution if Victor agree.

I don't know. If it doesn't get merged in mainline, I's not sure if it worths.
Comment 63 Nicolas Dufresne (ndufresne) 2017-08-10 14:42:34 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #62)
> (In reply to Nicolas Dufresne (stormer) from comment #61)
> 
> > On TI/Sitara branch, I notice they added a property to indicate to scale or
> > not. That could also be a shorter term solution if Victor agree.
> 
> I don't know. If it doesn't get merged in mainline, I's not sure if it
> worths.

I meant they added a property to kmssink, not a DRM property.
Comment 64 Víctor Manuel Jáquez Leal 2017-08-10 14:55:33 UTC
(In reply to Nicolas Dufresne (stormer) from comment #63)
> (In reply to Víctor Manuel Jáquez Leal from comment #62)
> > (In reply to Nicolas Dufresne (stormer) from comment #61)
> > 
> > > On TI/Sitara branch, I notice they added a property to indicate to scale or
> > > not. That could also be a shorter term solution if Victor agree.
> > 
> > I don't know. If it doesn't get merged in mainline, I's not sure if it
> > worths.
> 
> I meant they added a property to kmssink, not a DRM property.

Ah! sound interesting! Let's post the patch and we'll discuss it there :) (TI should do it, IMO).
Comment 65 Haihua Hu 2017-08-11 01:46:04 UTC
(In reply to Nicolas Dufresne (stormer) from comment #63)
> (In reply to Víctor Manuel Jáquez Leal from comment #62)
> > (In reply to Nicolas Dufresne (stormer) from comment #61)
> > 
> > > On TI/Sitara branch, I notice they added a property to indicate to scale or
> > > not. That could also be a shorter term solution if Victor agree.
> > 
> > I don't know. If it doesn't get merged in mainline, I's not sure if it
> > worths.
> 
> I meant they added a property to kmssink, not a DRM property.

Sound a good short term solution,I also hear about the atomic check for drm which can
 do configure only and don't do a real render. But I have no guide for how to use these api and there is also no demo code in my side. Do you have some resources for reference?
Comment 66 Haihua Hu 2017-08-11 15:52:30 UTC
(In reply to Nicolas Dufresne (stormer) from comment #63)
> (In reply to Víctor Manuel Jáquez Leal from comment #62)
> > (In reply to Nicolas Dufresne (stormer) from comment #61)
> > 
> > > On TI/Sitara branch, I notice they added a property to indicate to scale or
> > > not. That could also be a shorter term solution if Victor agree.
> > 
> > I don't know. If it doesn't get merged in mainline, I's not sure if it
> > worths.
> 
> I meant they added a property to kmssink, not a DRM property.

Hi Nicloas and Victor,

I create a atomic drm check patch for checking the scale capability of driver, please help review, I have pass test on my site with no scale driver, you can do more test. The patch use new atomic api to try the configuration with out rendering.
 
the thread is: https://bugzilla.gnome.org/show_bug.cgi?id=785663
Comment 67 Haihua Hu 2017-09-12 10:52:51 UTC
Created attachment 359611 [details] [review]
kmssink: add can-scale property
Comment 68 Haihua Hu 2017-09-12 10:53:22 UTC
Created attachment 359612 [details] [review]
kmssink: support videooverlay interface (version 5)
Comment 69 Haihua Hu 2017-09-12 10:56:27 UTC
Hi Nicloas and Victor,

Let's continue this work, please help test the new patch on your platform and give comments.

For short term solution, I add a property which can indicate the scale caps.

Thanks a lot.
Comment 70 Nicolas Dufresne (ndufresne) 2017-09-12 20:10:22 UTC
Review of attachment 359612 [details] [review]:

Just a note that the patch does not apply cleanly on git master.

::: sys/kms/gstkmssink.c
@@ +102,3 @@
+      width = self->hdisplay;
+      height = self->vdisplay;
+      goto commit;

You could avoid the goto here, and it would be nicer. Just check of -1, fix the width / height. And then check again if it's smaller/equal to zero and return.
Comment 71 Haihua Hu 2017-09-13 02:20:57 UTC
Created attachment 359676 [details] [review]
kmssink: support videooverlay interface (version 6)
Comment 72 Haihua Hu 2017-09-18 02:20:03 UTC
Any comments?
Comment 73 Víctor Manuel Jáquez Leal 2017-09-21 15:50:09 UTC
Review of attachment 359676 [details] [review]:

I have tested it with a minnowboard and my test looks good. I'll test it in a db410 now.

::: sys/kms/gstkmssink.c
@@ +152,3 @@
+    GST_OBJECT_UNLOCK (self);
+
+    gst_pad_push_event (GST_BASE_SINK (self)->sinkpad,

what about use 
GST_BASE_SINK_PAD(self)?
Comment 74 Víctor Manuel Jáquez Leal 2017-09-21 17:22:25 UTC
Tested it with my db410c using the ingeni (1920x1200) and my test app works as expected.

Besides of the nitpick of GST_BASE_SINK_PAD, it is ok to me.

What do you think Nicola?
Comment 75 Nicolas Dufresne (ndufresne) 2017-09-21 19:04:02 UTC
Review of attachment 359676 [details] [review]:

My impression is that this patch contains code that should have been added with the "can-scale" property patch. I think it overall looks better, but yet I think there is issues.

If you cannot scale, you should always prepend a fixed width/height caps in getcaps() ("can-scale" patch should have something). This provide upstream a recommended size, this is what ximagesink will do.

A bigger problem is the following. My screen is 1080p, I have a 100x100 pip that I'm displaying, my driver cannot scale. It's currently at 0,0, I want it to be at 200,200. I receive the renegotiation, which I ignore, because I don't want a new size. And finally the render rectangle is never updated. I think we should apply the rectangle immediately when the size does not change. In fact, I would always apply the rect immediatly and crop by default. Delaying the rect should maybe be an option as it's behaviour can be surprising. An example, it does not take effect if paused.

::: sys/kms/gstkmssink.c
@@ +111,3 @@
+  GST_OBJECT_LOCK (self);
+  if (self->can_scale) {
+    self->preferred_rect.x = x;

I'd rename this one render_rect. It's not just a preference, this is the size we are currently rendering to.
Comment 76 Nicolas Dufresne (ndufresne) 2017-09-21 20:26:35 UTC
Comment on attachment 356481 [details]
test app

Was confused by this test, has you pass width/height to -1, the x/y is ignored. This seems totally correct form the sink, since the doc says:

"To unset the region pass -1 for the width and height parameters."

But that's also why we need a window-width/height (probably display-width/height for kms ?) readable property, in order to be able to clip from an application. These prop exist in ximagesink and xvimagesink.
Comment 77 Nicolas Dufresne (ndufresne) 2017-09-21 20:33:15 UTC
Be aware that the speculated bug of not getting a new caps event if width/height didn't change can be reproduced with the test app. I have fixed the width/height to 100x100 and set the can-scale property to TRUE. In this case, the video window never moves.
Comment 78 Nicolas Dufresne (ndufresne) 2017-09-21 20:45:14 UTC
Review of attachment 359676 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +153,3 @@
+
+    gst_pad_push_event (GST_BASE_SINK (self)->sinkpad,
+        gst_event_new_reconfigure ());

I didn't realize, but this code is not correct. expose should always do a synchronous _show_frame(), even if the new render rectangle is not yet applied. You have an old last buffer in hand anyway.
Comment 79 Víctor Manuel Jáquez Leal 2017-09-22 10:42:45 UTC
Created attachment 360251 [details]
kms overlay test app (v2)

oops! Sorry, I forgot to update the test app.
Comment 80 Víctor Manuel Jáquez Leal 2017-09-29 10:07:55 UTC
I propose this steps:

1. A patch for the videooverlay without any scaling logic, the simplest approach. Let's the kernel complain if the rectangle is out of its framebuffer.

2. The patch for scaling detection (bug 785663)

3. The patch for can-scale property (this might be not required at the end)

4. If the kernel driver can scale, complete the patch set with one allowing the scaling at expose() if the ovelay area is different from the frame one.

Thus, at least we'll have the videooverlay feature merged soon.

What do you think?
Comment 81 Nicolas Dufresne (ndufresne) 2017-09-29 13:29:37 UTC
1. I'm find with this, at least it would create a base for the scaling drivers.

2. Requires atomic KMS, which may not be supported, hence 3 can help. I'm still not fan of mixing up atomic/non-atomic. I still think a new sink for using atomic should be added, so we don't create a large mess.

To be fair, if the patchset was tested properly, it would be merged already.
Comment 82 Haihua Hu 2017-10-09 08:01:03 UTC
(In reply to Nicolas Dufresne (stormer) from comment #81)
> 1. I'm find with this, at least it would create a base for the scaling
> drivers.
> 
> 2. Requires atomic KMS, which may not be supported, hence 3 can help. I'm
> still not fan of mixing up atomic/non-atomic. I still think a new sink for
> using atomic should be added, so we don't create a large mess.
> 
> To be fair, if the patchset was tested properly, it would be merged already.

Sorry to late reply this thread. So you mean we need create a scaling videooverlay base, and then add non-scaling handle? This will separate this feature into at least two patch, right?
Comment 83 Wonchul Lee 2017-11-13 13:51:25 UTC
Hi, As the other experts have said above, It seems to be able to proceed with the 1 + 3 patches here. Then we could consider atomic KMS by creating another KMSsink or another way if possible on bug 785663.
As they pointed out earlier, you could just replace GST_BASE_SINK (self)->sinkpad to GST_BASE_SINK_PAD (self) and fix to ensure show_frame() on expose () in either case. It would be getting good, IMHO.
Comment 84 Wonchul Lee 2017-11-15 06:31:33 UTC
Created attachment 363659 [details] [review]
kmssink: support videooverlay interface (version 7)

I attached a patch which is based on Haihua's work because I just wanted to make it adapted to upstream quickly.
I've fixed to update the render_rect when the rectangle size isn't changed, only the position (x,y) changed, it will not renegotiate which doesn't affect in this case and just updated final render_rect, also fixed some simple things mentioned earlier.
Comment 85 Haihua Hu 2017-11-15 06:46:00 UTC
(In reply to Wonchul Lee from comment #84)
> Created attachment 363659 [details] [review] [review]
> kmssink: support videooverlay interface (version 7)
> 
> I attached a patch which is based on Haihua's work because I just wanted to
> make it adapted to upstream quickly.
> I've fixed to update the render_rect when the rectangle size isn't changed,
> only the position (x,y) changed, it will not renegotiate which doesn't
> affect in this case and just updated final render_rect, also fixed some
> simple things mentioned earlier.

Thanks Wonchul,

You change look good for me.
Comment 86 Víctor Manuel Jáquez Leal 2017-11-15 11:48:10 UTC
(In reply to Wonchul Lee from comment #84)
> Created attachment 363659 [details] [review] [review]
> kmssink: support videooverlay interface (version 7)
> 
> I attached a patch which is based on Haihua's work because I just wanted to
> make it adapted to upstream quickly.
> I've fixed to update the render_rect when the rectangle size isn't changed,
> only the position (x,y) changed, it will not renegotiate which doesn't
> affect in this case and just updated final render_rect, also fixed some
> simple things mentioned earlier.

I haven't tested it, but it looks cleaner.
Comment 87 Wonchul Lee 2017-12-01 06:48:00 UTC
Created attachment 364716 [details] [review]
kmssink: support videooverlay interface (version 7)

There is no change in the patch, I just fixed to keep the original author of this patch
Comment 88 Nicolas Dufresne (ndufresne) 2017-12-01 17:02:04 UTC
Created attachment 364767 [details] [review]
kmssink: Add display-width/height properties

This is to be used with gst_video_overlay_set_render_rectangle()
so the application can calculate a rectangle that fits inside
the display. The property changes are notify in a way that you
can watch either notify::display-width or notify::display-height
and both will be up-to-data when this is called back. Before the
element is started, the size will be 0x0.
Comment 89 Nicolas Dufresne (ndufresne) 2017-12-01 17:03:31 UTC
Created attachment 364768 [details]
Updated test app with display-width/height support

I just updated the test to demonstrate that display-width/height works.
Comment 90 Nicolas Dufresne (ndufresne) 2017-12-01 17:04:05 UTC
Attachment 359611 [details] pushed as ff9a439 - kmssink: add can-scale property
Attachment 364767 [details] pushed as 737067e - kmssink: Add display-width/height properties
Comment 91 Nicolas Dufresne (ndufresne) 2017-12-01 17:04:57 UTC
Comment on attachment 364716 [details] [review]
kmssink: support videooverlay interface (version 7)

Git bz failed picking it up, so:

commit b24bb73101be06f48a49826fc7b7f8d418173e26
Author: Haihua Hu <jared.hu@nxp.com>
Date:   Thu Sep 14 18:12:18 2017 +0800

    kmssink: support videooverlay interface
    
    Implement videooverlay interface in kmssink, divided into two cases:
    when driver supports scale, then we do refresh in show_frame(); if
    not, send a reconfigure event to upstream and re-negotiate, using the
    new size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=784599
Comment 92 Nicolas Dufresne (ndufresne) 2017-12-01 17:07:04 UTC
Thanks. I'll try to make this even more useful in a near feature, adding GstContext support, so we can have multiple kmssink running, and also the application could handle one or more graphic plane for it's UI.
Comment 93 Víctor Manuel Jáquez Leal 2017-12-01 17:34:01 UTC
Review of attachment 364716 [details] [review]:

::: sys/kms/gstkmssink.c
@@ +813,3 @@
+
+    gst_video_calculate_device_ratio (self->hdisplay, self->vdisplay,
+        self->mm_width, self->mm_height, &dpy_par_n, &dpy_par_d);

This function call looks like dead code to me. It is not clear to me what's its purpose.
Comment 94 Víctor Manuel Jáquez Leal 2017-12-01 17:46:26 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #93)
> Review of attachment 364716 [details] [review] [review]:
> 
> ::: sys/kms/gstkmssink.c
> @@ +813,3 @@
> +
> +    gst_video_calculate_device_ratio (self->hdisplay, self->vdisplay,
> +        self->mm_width, self->mm_height, &dpy_par_n, &dpy_par_d);
> 
> This function call looks like dead code to me. It is not clear to me what's
> its purpose.

or is missing the PAR in the new caps' structure
Comment 95 Nicolas Dufresne (ndufresne) 2017-12-01 18:51:54 UTC
Damn, missed it. This code is completely wrong. The reconfigure trigger not right. If we cannot scale, then we should force the pixel-aspect-ratio to the display pixel-aspect ratio and (unless the display width/height is not yet known) we should ALWAYS *prepand* that size. I'll rewrite that part.
Comment 96 Nicolas Dufresne (ndufresne) 2017-12-01 21:56:17 UTC
Created attachment 364790 [details] [review]
kmssink: Enforce pixel aspect ratio when we cannot scale

When we cannot scale, we need to enforce the pixel aspect ratio.
This was partly implemented in the previous patch. Doing this
simplify some of the code.
Comment 97 Nicolas Dufresne (ndufresne) 2017-12-01 22:00:37 UTC
Ok, I'm a bit dumping a patch here, but this is a quick rework to handle aspect ratio correctly. Basically, when we don't scale, we should not accept pixel-aspect-ratio that don't match the display. As a side effect, I could remove and simplify some code all around.

For those testing on Intel like me, be aware that the scaler may have serious limitation, it does not fail and render a cropped image that is wrong. For my case, scaling down is very limited, while scaling up seems to do anything. E.g. It fails if I try to render 4K, it simply crop it.
Comment 98 Nicolas Dufresne (ndufresne) 2017-12-06 19:14:58 UTC
Attachment 364790 [details] pushed as d33aff0 - kmssink: Enforce pixel aspect ratio when we cannot scale