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 760479 - basesrc: Only respond to duration/position queries if the quantity is valid
basesrc: Only respond to duration/position queries if the quantity is valid
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-11 16:34 UTC by Carlos Rafael Giani
Modified: 2016-01-11 20:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix for basesrc duration&position queries (1.92 KB, patch)
2016-01-11 16:34 UTC, Carlos Rafael Giani
none Details | Review
fix for basesrc duration&position queries v2 (1.35 KB, patch)
2016-01-11 20:19 UTC, Carlos Rafael Giani
committed Details | Review

Description Carlos Rafael Giani 2016-01-11 16:34:04 UTC
Created attachment 318765 [details] [review]
fix for basesrc duration&position queries

Currently, basesrc can set invalid value combinations in the duration and
position queries.

Example: duration query uses the TIME format, the basesrc only knows a
duration in BYTES of 400000 bytes, and conversion fails. However, prior to
this change, the code still answers the query, setting the format to TIME,
and the duration to 400000. This misleads downstream elements who first ask
upstream if it knows the duration in TIME format; they misinterpret the
value 400000 (which is actually the size in bytes) as a duration in TIME.

With this patch, the queries are answered only if the duration/position
value is actually valid. Downstream elements then simply get a failed
query, as opposed to a "successful" but actually invalid one.
Comment 1 Sebastian Dröge (slomo) 2016-01-11 16:52:58 UTC
Review of attachment 318765 [details] [review]:

Makes sense but why would the query be "successful" right now? res is FALSE, so gst_pad_query() should return FALSE. The query only contains invalid values afterwards
Comment 2 Carlos Rafael Giani 2016-01-11 17:26:03 UTC
Actually, that is an excellent question :) I dug further, and as it turns out, there was one custom sink element in the application here that didn't check the gst_element_query() return value, assuming that it always succeeds. Still, I think the patch is valid. I'll update the commit message.
Comment 3 Sebastian Dröge (slomo) 2016-01-11 17:35:40 UTC
I think so too, it should be merged with a more correct commit message :)
Comment 4 Carlos Rafael Giani 2016-01-11 20:19:46 UTC
Created attachment 318822 [details] [review]
fix for basesrc duration&position queries v2
Comment 5 Sebastian Dröge (slomo) 2016-01-11 20:43:31 UTC
commit 4b0db9fd2412c1102d3c22dece1243fe441ac3f8
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Mon Jan 11 21:17:25 2016 +0100

    basesrc: Only set duration/position query values in case of query success
    
    Currently, the query values are being set even if the query itself was
    determined to have failed. Fix this to ensure the values are only set in
    case of a query success.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760479