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 758717 - bayer: pixel aspect ratio not forwarded when transforming caps
bayer: pixel aspect ratio not forwarded when transforming caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.10.0
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-26 18:48 UTC by Joan Pau
Modified: 2016-10-31 14:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
bayer: Forward pixel aspect ratio when transforming caps in bayer elements (3.05 KB, patch)
2015-11-26 18:55 UTC, Joan Pau
none Details | Review
proper fix of _transform_caps method (5.15 KB, patch)
2016-10-13 14:12 UTC, Joan Pau
committed Details | Review

Description Joan Pau 2015-11-26 18:48:41 UTC
Negotiation errors may arise when bayer elements are present in the pipeline,
because they do not forward the pixel aspect ratio when transforming the caps:

    # Failure due to the combination of `bayer2rgb` and `videoscale`:
    gst-launch-1.0 -v \
      videotestsrc \
      ! "video/x-bayer,format=rggb,width=1280,height=960,framerate=15/2" \
      ! bayer2rgb \
      ! videoscale \
      ! "video/x-raw,framerate=15/2,width=640,pixel-aspect-ratio=1/1" \
      ! ximagesink
Comment 1 Joan Pau 2015-11-26 18:55:30 UTC
Created attachment 316337 [details] [review]
bayer: Forward pixel aspect ratio when transforming caps in bayer elements
Comment 2 Joan Pau 2016-10-13 14:12:31 UTC
Created attachment 337609 [details] [review]
proper fix of _transform_caps method

The previous patch was a quick and dirty solution to the issue.
This one contains the proper implementation of the _transform_caps method.
Comment 3 Nicolas Dufresne (ndufresne) 2016-10-13 15:09:40 UTC
Review of attachment 337609 [details] [review]:

Except for the set_name() part, it looks correct to me.

::: gst/bayer/gstbayer2rgb.c
@@ +299,3 @@
+    structure = gst_caps_get_structure (res_caps, i);
+    if (direction == GST_PAD_SINK) {
+      gst_structure_set_name (structure, "video/x-raw");

That one is not needed, since your caps template will ensure that already.

@@ +302,3 @@
+      gst_structure_remove_field (structure, "format");
+    } else {
+      gst_structure_set_name (structure, "video/x-bayer");

And same.

@@ +304,3 @@
+      gst_structure_set_name (structure, "video/x-bayer");
+      gst_structure_remove_fields (structure, "format", "colorimetry",
+          "chroma-site", NULL);

Curiosity (not a review comment), do we just ignore these and do we actually handle it in the conversion already ?
Comment 4 Nicolas Dufresne (ndufresne) 2016-10-13 15:34:57 UTC
Review of attachment 337609 [details] [review]:

Arg, and I'm confused again, sorry. Obviously you need to change from raw to bayer (and vis-versa). I only write those for raw to raw conversion in general, so that's why it felt unfamiliar.
Comment 5 Joan Pau 2016-10-14 09:28:30 UTC
So is it ready to merge as it is?
Or there is something missing?
Comment 6 Nicolas Dufresne (ndufresne) 2016-10-14 13:44:14 UTC
I'm on it, just ran out of time yesterday, just need to merge, build and re-test. So if nothing happens, should be in before 1.10 ;-P