GNOME Bugzilla – Bug 756211
Porting gst_video_sink_center_rect to gst_util_uint64_scale_int
Last modified: 2018-05-06 13:05:29 UTC
So far, gst_video_sink_center_rect had only a few fixed points. More specifically, the returned destination rectangle was equal to the input rectangle if and only if the src and dst scaling ratios were identical. The current commit changes the implementation in the way that for every dst height there exists a corresponding width, such that the resulting rectangle constitutes a fixed point. Furthermore, this rectangle has an aspect ratio that deviates at most one pixel from a rectangle with the "true" source aspect ratio. One advantage of this change is that gstreamer applications may use the fixed points in order to strip black margins around videos.
Created attachment 312863 [details] [review] patch
Comment on attachment 312863 [details] [review] patch Thanks, can you provide unit tests for this to make sure it behaves the same as before for the cases that worked before? Check in tests/check
Unfortunately, I am lacking time and probably also knowledge to dig deeper into the code here to provide the corresponding unit tests. Nonetheless, checking with "make check" in the tests/check subdirectory shows that 61 of 61 unit tests pass. In order to increase confidence into the provided code, it should be noted that it is in fact very similar to what gstreamer-vaapi does: https://github.com/gbeauchesne/gstreamer-vaapi/blob/1b0ced484e5fd6f2a291c282934024e1c513f1bf/gst/vaapi/gstvaapisink.c#L1019
Created attachment 316673 [details] [review] tests: video: add tests for video_center_rect
I was looking for real case examples of video_center_rect but I could not find "strange" use cases. But, I did add a simple profiling (after and before gst_util_get_timestamp ()) and compared the numbers with this patch. Running this command $ LOOPS=100 CK_LOG_FILE_NAME=- CK_VERBOSITY=verbose CK_FORK=no GST_CHECKS=test_video_center_rect make libs/video.torture and getting an average, the current algorithm is slightly better than this patch in my hardware (Haswell i7).
What do you mean with better? The new version is going to be slower, but the goal here is that the new version should be more accurate and give more correct results
(In reply to Sebastian Dröge (slomo) from comment #6) > What do you mean with better? The new version is going to be slower, but the > goal here is that the new version should be more accurate and give more > correct results The average of my samplings (unit test) is: 47895.65 nanoseconds with the patch 46005.29 nanoseconds without the patch Thinking about, perhaps we should check the aspect ratio after the transformation, it should be the same.
We should also write tests for the specific cases that now work properly but didn't before :)
> Thinking about, perhaps we should check the aspect ratio after the > transformation, it should be the same. Víctor, are you going to add that to your unit test patch? > We should also write tests for the specific cases that now work > properly but didn't before :) Philipp, do you have an example where the code didn't do the right thing before your patch, so we can use that in the unit test?
If I remember correctly I experienced the issue while tinkering with Okular's video rendering, which afaik is done via the Gstreamer Phonon backend. I guess that this is probably not the unit test you are looking for :)
I was looking for specific input values that produce wrong output :)
Ah sry, I'll probably do not have the exact values any more.
If this is not reproducible anymore, I'll close it as INCOMPLETE. Please feel free to reopen it if you can provide a way to reproduce it. Thanks!