GNOME Bugzilla – Bug 784599
kmssink: support videooverlay interface
Last modified: 2017-12-06 19:15:03 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.
Created attachment 354995 [details] [review] kmssink: support videooverlay interface
Created attachment 355004 [details] [review] kmssink: support videooverlay interface
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 on attachment 355004 [details] [review] kmssink: support videooverlay interface Same comments apply I believe.
*** Bug 765815 has been marked as a duplicate of this bug. ***
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
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?
Also I would try to factor out the math to keep the lock logic isolated.
Thanks for your comments, I am refining my patch and will submit today.
Created attachment 355313 [details] [review] patch refined
Review of attachment 355313 [details] [review]: Just a note, set window handle is not implement, what happens if we call this ?
(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 }
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.
Hi all, Any updates?
Review of attachment 355313 [details] [review]: Sorry, but first you need to rebase the patch :(
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 ()
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.
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
(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?
(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.
Created attachment 356413 [details] [review] kmssink video-overlay interface version 3
(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.
Review of attachment 356413 [details] [review]: looks good to me. Any comment @Nicolas?
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 ?
(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).
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.
(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 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.
(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.
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.
Created attachment 356481 [details] test app This a simple test app that moves the output of videotestsrc in a diagonal across the screen.
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)
(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.
(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.
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>
Review of attachment 356413 [details] [review]: mark this as reject to continue reviewing the next version of the patch
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.
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;
(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?
Created attachment 356624 [details] [review] kmssink: support videooverlay interface (version 4)
(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.
Attachment 356483 [details] pushed as db07f45 - kmssink: support videooverlay interface
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.
(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?
Oh, sorry, I'm not watching this ML these days. I meant that someone should ask.
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
This patch causes regression on MSM DRM (DB410c). We get ENOSPC, kernel error: Invalid source coordinates 320.000000x264.000000+0.000000+0.000000
(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?
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.
(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?
Created attachment 356870 [details] Log for the ENOSPC regressions
Notice, it says "scaling to 320x264", which seems wrong.
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.
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.
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 ?
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.
(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.
(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
(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).
(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).
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.
(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.
(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.
(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).
(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?
(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
Created attachment 359611 [details] [review] kmssink: add can-scale property
Created attachment 359612 [details] [review] kmssink: support videooverlay interface (version 5)
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.
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.
Created attachment 359676 [details] [review] kmssink: support videooverlay interface (version 6)
Any comments?
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)?
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?
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 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.
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.
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.
Created attachment 360251 [details] kms overlay test app (v2) oops! Sorry, I forgot to update the test app.
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?
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.
(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?
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.
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.
(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.
(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.
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
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.
Created attachment 364768 [details] Updated test app with display-width/height support I just updated the test to demonstrate that display-width/height works.
Attachment 359611 [details] pushed as ff9a439 - kmssink: add can-scale property Attachment 364767 [details] pushed as 737067e - kmssink: Add display-width/height properties
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
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.
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.
(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
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.
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.
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.
Attachment 364790 [details] pushed as d33aff0 - kmssink: Enforce pixel aspect ratio when we cannot scale