[Bug 790149] New: vaapipostproc: Does not maintain aspect ratio when scaling

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Bug ID: 790149
Summary: vaapipostproc: Does not maintain aspect ratio when
scaling
Classification: Platform
Product: GStreamer
Version: git master
OS: Linux
Status: NEW
Severity: normal
Priority: Normal
Component: gstreamer-vaapi
Assignee: gstreamer-***@lists.freedesktop.org
Reporter: ***@ndufresne.ca
QA Contact: gstreamer-***@lists.freedesktop.org
CC: ***@gmail.com, ***@igalia.com
GNOME version: ---

When vaapipostproc is being used (notably through vaapidecodebin), the
vaapipostproc may endup scaling the video. The problem is that the scaling does
not maintain the aspect ratio, which is totally unexpected default behaviour.
On can illustrace this with a non scaling display sink.

gst-launch-1.0 videotestsrc ! video/x-raw,width=320,height=240 ! vaapipostproc
! ximagesink

In comparison with:

gst-launch-1.0 videotestsrc ! video/x-raw,width=320,height=240 ! videoscale !
ximagesink

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #1 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
I don't deny there might be a problem handling the display-aspect-ratio with
vaapipostproc, but I can't reproduce it, in my setup, with those examples.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #2 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
I didn't check yet, but Wich layer of abstraction take care of that ? If it's
not reliable it might be something inside the driver, with different bug
depending on the driver version ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #3 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
oops! I can reproduce it!

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Julien Isorce <***@gmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #4 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Just to keep the ball rolling, can vaapipostproc add black bars, top/bottom or
left/right depending ?

I'm asking because scalers have two options:

a) they maintain aspect ratio by negotiating output caps that do respect it
b) scale to the negotiated output size, and add black borders as needed

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #5 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Any answer to my question ?

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #6 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #4)

Post by "GStreamer" (GNOME Bugzilla)
Just to keep the ball rolling, can vaapipostproc add black bars, top/bottom
or left/right depending ?
a) they maintain aspect ratio by negotiating output caps that do respect it
b) scale to the negotiated output size, and add black borders as needed

Honestly, I don't know for sure, I've never tried. But, reading the
gstvaapifilter code, and the VA-API, it seems possible to do option b):

https://cgit.freedesktop.org/gstreamer/gstreamer-vaapi/tree/gst-libs/gst/vaapi/gstvaapifilter.c#n1444

Since it is possible to set a destine rectangle with a specified background
color.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #9 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 367038
--> https://bugzilla.gnome.org/attachment.cgi?id=367038&action=edit
vaapipostproc: keep the display's pixel aspect ratio

Before vaapipostproc will resize the video frame to the window size,
but that broke the display's pixel aspect ratio form the original
video.

This patch will honor the original pixel-aspect-ratio by setting a
target rectangle within the output surface, that means black borders.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #10 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
(In reply to Víctor Manuel Jáquez Leal from comment #9)

Created attachment 367038 [details] [review]
vaapipostproc: keep the display's pixel aspect ratio
Before vaapipostproc will resize the video frame to the window size,
but that broke the display's pixel aspect ratio form the original
video.
This patch will honor the original pixel-aspect-ratio by setting a
target rectangle within the output surface, that means black borders.

This patch works regular with intel-vaapi-driver, it shows some artifacts and
the image is not centered.

In the case of mesa amd driver, it is worse since it crashes.

Dunno, perhaps it is a code path no quite tested by drivers. Still, it seems
the correct way to do it.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #8 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 367037
--> https://bugzilla.gnome.org/attachment.cgi?id=367037&action=edit
vaapipostproc: remove spurious code

This assignation is dead code, since gst_video_info_from_caps() set
to 1 by default.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #7 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Created attachment 367036
--> https://bugzilla.gnome.org/attachment.cgi?id=367036&action=edit
vaapipostproc: if no p-a-r in out caps define a range

Instead of copying the pixel-aspect-ratio from the sink caps, define
an open range for the src caps pixel-aspect-ratio. Later it will be
defined.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #367036|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Víctor Manuel Jáquez Leal <***@igalia.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #367037|none |committed
status| |

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #11 from Víctor Manuel Jáquez Leal <***@igalia.com> ---
Attachment 367036 pushed as ea9c52e - vaapipostproc: if no p-a-r in out caps
define a range
Attachment 367037 pushed as da77fd5 - vaapipostproc: remove spurious code

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #367038|none |accepted-commit_now
status| |

--- Comment #12 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 367038:
--> (https://bugzilla.gnome.org/review?bug=790149&attachment=367038)

I think we should get that in before 1.14.

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

--- Comment #13 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Ok, after testing more myself, I agree with you, we'll need to debug the
driver, I had a complete system hang and had to reboot:

vainfo: VA-API version: 0.40 (libva )
vainfo: Driver version: Intel i965 driver for Intel(R) Ivybridge Mobile - 1.8.3

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

Nicolas Dufresne (stormer) <***@ndufresne.ca> changed:

What |Removed |Added
----------------------------------------------------------------------------
Attachment #367038|accepted-commit_now |reviewed
status| |

--- Comment #14 from Nicolas Dufresne (stormer) <***@ndufresne.ca> ---
Review of attachment 367038:
--> (https://bugzilla.gnome.org/review?bug=790149&attachment=367038)

Again, I don't see anything ovbiously wrong in the this code though ...

["GStreamer" (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=790149

GStreamer system administrator <***@gstreamer.net> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |OBSOLETE

--- Comment #15 from GStreamer system administrator <***@gstreamer.net> ---
-- 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/gstreamer-vaapi/issues/74.