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 724893 - playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-02-21 14:09 UTC by Matthieu Bouron
Modified: 2014-02-26 08:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return (2.13 KB, patch)
2014-02-21 14:13 UTC, Matthieu Bouron
needs-work Details | Review
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return (2.92 KB, patch)
2014-02-24 10:13 UTC, Matthieu Bouron
needs-work Details | Review
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return (2.80 KB, patch)
2014-02-25 13:10 UTC, Matthieu Bouron
committed Details | Review

Description Matthieu Bouron 2014-02-21 14:09:49 UTC
The following patch makes gst_play_sink_convert_bin_getcaps return:
  peer_caps + intersect_first (filter, converter_caps)
instead of
  intersect_first (filter, peer_caps + converter_caps)
so it preservers downstream caps preference order.
Comment 1 Matthieu Bouron 2014-02-21 14:13:29 UTC
Created attachment 269922 [details] [review]
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return
Comment 2 Sebastian Dröge (slomo) 2014-02-22 15:19:07 UTC
Comment on attachment 269922 [details] [review]
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return

This is not 100% correct yet. "converter_caps" might be bigger than the filter caps.

What you need to do is to return
intersect_first(filter, peer_caps) + intersect_first(filter, converter_caps)

Also I think the intersection part should just be moved from the bottom inside the if (otherpad) branch and its else branch instead of adding this "apply_filter" variable.
Comment 3 Sebastian Dröge (slomo) 2014-02-22 15:20:19 UTC
(In reply to comment #2)
> (From update of attachment 269922 [details] [review])
> This is not 100% correct yet. "converter_caps" might be bigger than the filter
> caps.
> 
> What you need to do is to return
> intersect_first(filter, peer_caps) + intersect_first(filter, converter_caps)

Nevermind, ignore that part. It's correct as is, the peer_caps are already filtered.

But please make the code a bit clearer by moving the filtering inside the if/else blocks.
Comment 4 Matthieu Bouron 2014-02-24 10:13:12 UTC
Created attachment 270101 [details] [review]
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return

Patch updated. I moved the filtering part to each relevant if else blocks.
Comment 5 Sebastian Dröge (slomo) 2014-02-25 09:02:41 UTC
Review of attachment 270101 [details] [review]:

::: gst/playback/gstplaysinkconvertbin.c
@@ +395,3 @@
+              self->converter_caps, GST_CAPS_INTERSECT_FIRST);
+        }
+        ret = gst_caps_merge (peer_caps, converter_caps);

Why don't you use the macro here too?

@@ +406,3 @@
   } else {
     ret = gst_caps_new_any ();
+    GST_PLAY_SINK_CONVERT_BIN_FILTER_CAPS (filter, ret);

ret = filter ? gst_caps_ref (filter) : gst_caps_new_any ();
Comment 6 Matthieu Bouron 2014-02-25 13:10:40 UTC
Created attachment 270267 [details] [review]
playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return

Patch updated taking into account your latest comments. Thanks for the review.
Comment 7 Sebastian Dröge (slomo) 2014-02-26 08:35:30 UTC
commit 5c1167a2c7eff6fb370176ae4541d4e670d65cab
Author: Matthieu Bouron <matthieu.bouron@collabora.com>
Date:   Fri Feb 21 14:01:37 2014 +0000

    playsinkconvertbin: improve gst_play_sink_convert_bin_getcaps return
    
    If we have the peer caps and a caps filter, return peer_caps +
    intersect_first (filter, converter_caps) instead of
    intersect_first (filter, peer_caps + converter_caps) and preservers
    downstream caps preference order.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=724893