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 761458 - adaptivedemux: Unused GstSegment variable in download loop
adaptivedemux: Unused GstSegment variable in download loop
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-02 14:41 UTC by David Waring
Modified: 2016-02-05 13:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Option 1: Use the local variable to create the new segment message. (1.07 KB, patch)
2016-02-02 14:41 UTC, David Waring
none Details | Review
Option 2: Remove local variable and use stream->segment instead. (1.62 KB, patch)
2016-02-02 14:42 UTC, David Waring
none Details | Review
Option 3: Like option 2 but don't reinitialise from demux->segment. (1.56 KB, patch)
2016-02-02 14:43 UTC, David Waring
committed Details | Review

Description David Waring 2016-02-02 14:41:13 UTC
Created attachment 320265 [details] [review]
Option 1: Use the local variable to create the new segment message.

During processing in gst_adaptive_demux_stream_download_loop, if the stream->restart_download boolean is set, a local GstSegment variable, segment, is created and updated with a new offset, but is then never actually used. Instead stream->segment is used to send a new segment event.

This looks wrong, so I suspect either the local variable shouldn't be there and stream->segment should be used instead or the local variable should be there and should be the one used in the new segment event.

However I'm unsure as to exactly what this ought to look like, so here are 3 patches which are my guesses at what the code should look like.

1. Use the local variable to create the new segment message.
2. Remove the local segment variable and just use stream->segment instead.
3. Like 2 but also don't reinitialise stream->segment from demux->segment.

Could someone who understands what the code should be doing have a look at this bit of the code?
Comment 1 David Waring 2016-02-02 14:42:34 UTC
Created attachment 320266 [details] [review]
Option 2: Remove local variable and use stream->segment instead.
Comment 2 David Waring 2016-02-02 14:43:23 UTC
Created attachment 320267 [details] [review]
Option 3: Like option 2 but don't reinitialise from demux->segment.
Comment 3 Thiago Sousa Santos 2016-02-05 13:50:00 UTC
Merged option 3, thanks.

It shouldn't be needed to reinitialize the segment as it is kept in sync when it changes via a seek.

commit 90fe6c5a5bdba98a51c17674f0e6c38cec04f7da
Author: David Waring <david.waring@rd.bbc.co.uk>
Date:   Tue Feb 2 13:50:25 2016 +0000

    adaptivedemux: Update position in stream->segment for new stream segment message.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=761458