GNOME Bugzilla – Bug 748107
tsdemux: fix build error with clang
Last modified: 2016-08-08 17:32:57 UTC
There is a build error when tsdemux is compiled with clang. bash-3.2$ make CC libgstmpegtsdemux_la-tsdemux.lo tsdemux.c:2245:31: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare] if (demux->segment.position < 0) ~~~~~~~~~~~~~~~~~~~~~~~ ^ ~ 1 error generated. make: *** [libgstmpegtsdemux_la-tsdemux.lo] Error 1
Created attachment 301903 [details] [review] tsdemux: fix build error with clang This commit avoids a build error by clang with [-Werror,-Wtautological-compare] options.
Review of attachment 301903 [details] [review]: It seems to be that this will miss all the interesting cases (and be a noop anyway). I think this would be better: if (GST_BUFFER_PTS (buffer) > stream->first_dts) demux->segment.position = GST_BUFFER_PTS (buffer) - stream->first_dts; else demux->segment.position = 0;
Created attachment 302015 [details] [review] tsdemux: prevent negative value for position of GstSegment tsdemux: prevent negative value for position of GstSegment This is a new patch to prevent negative value for position of GstSegment.
Looks like Luis "fixed" this already by just removing it as well..
The patch by Luis seems to be intended to remove dead code because an unsigned int64 variable cannot have negative value. But this patch is intended to prevent the segment position from being assigned a negative value. Of course, if the result of segment position calculation is always right, this patch will be useless.
(In reply to Tim-Philipp Müller from comment #4) > Looks like Luis "fixed" this already by just removing it as well.. http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=f52cf2a7c4a03065eb435d9df390923783f4c715 (In reply to Yujin Lee from comment #5) > But this patch is intended to prevent the segment position from being assigned a negative value. In your version, it'll just be a noop. (We certainly don't mean to make the "negative" half of the position values invalid.) (In reply to Vincent Penquerc'h from comment #2) > if (GST_BUFFER_PTS (buffer) > stream->first_dts) > demux->segment.position = GST_BUFFER_PTS (buffer) - stream->first_dts; > else > demux->segment.position = 0; Something is seriously broken if that happens (as first_dts <= dts1 <= pts1 < dts2 <= pts2 ...). If we believe such a scenario is likely, and want to be defensive, I wonder if we're better logging the error and dropping the buffer, instead of silently propagating the error in the form of a sudden position drop. If such a scenario is unlikely, this bug can be closed.
For the record, first_dts the well named was either a pts or dts. It looks like the code was changed since though. Iit's now called first_pts, which can now be a pts or dts.
Is this still an issue with git master? It looks like the code is different now, I don't see any subtractions to guard against any more. Please re-open if it's still there and I missed it.