GNOME Bugzilla – Bug 762986
videoscale: Add option for navigation events to scale and respect internal region without borders
Last modified: 2018-11-03 11:45:16 UTC
The current version of videoscale scales navigation events based on the input to output ratio. If the output includes borders, the scaling includes the border area, which causes navigation events to be misaligned compared to the position of the mouse. This can be illustrated with the following example: gst-launch-1.0 videotestsrc ! navigationtest ! video/x-raw,width=1024,height=512 ! videoscale ! videoconvert ! ximagesink If the output window is resized to include borders, moving the mouse over the output This patch provides a fix for this behavior, scaling the navigation events based on only the internal region and not the borders. All events that are within the borders are discarded.
Created attachment 322851 [details] [review] Patch for fixing navigation handling
Created attachment 322852 [details] [review] Optional patch for selecting between old and new behaviour
I have added a secondary patch for selecting between old and new behavior. This is intended to maintain backwards compatibility if required
I think the old behaviour makes no sense. The translation should always be in a way that corresponds to the same position before and after.
Review of attachment 322851 [details] [review]: Makes sense, ideally can you also provide a unit test? There should already be one that is testing navigation events, would just have to be extended a bit. ::: gst/videoscale/gstvideoscale.c @@ +1114,3 @@ + gst_structure_set (structure, "pointer_x", G_TYPE_DOUBLE, + ((a - videoscale->borders_w / 2) * filter->in_info.width / + (filter->out_info.width - videoscale->borders_w)), NULL); This one degenerates into the one below for borders_w == 0. So maybe just have a single gst_structure_set() here :) Should simplify the code a bit @@ +1127,3 @@ + gst_structure_set (structure, "pointer_y", G_TYPE_DOUBLE, + ((a - videoscale->borders_h / 2) * filter->in_info.height / + (filter->out_info.height - videoscale->borders_h)), NULL); Same here
Created attachment 323000 [details] [review] Rewored patch for fixing navigation event handling
Created attachment 323001 [details] [review] Unit test for checking new navigation event handling
Comment on attachment 323001 [details] [review] Unit test for checking new navigation event handling I would refactor that pipeline setup into a function that takes arguments for the caps, etc so you can test with different resolutions and borders, and an array of navigation event positions and expected results It would make sense to test at least the no-borders case too here
Created attachment 323273 [details] [review] Added a bugfix for exclusion of an edge case I just merged a minor bug fix with the patch for videoscale. I was not handling the case when the target width or height is equal to the border_w/_h / 2, which are valid navigation events and equal to 0.
Created attachment 323274 [details] [review] Implemented the suggested changes for multiple testcases defined in arrays and adding the zero border case
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/257.