GNOME Bugzilla – Bug 779957
souphttpsrc: Manipulate range header when seek to 0
Last modified: 2017-11-10 09:25:51 UTC
Created attachment 347778 [details] [review] souphttpsrc: Manipulate range header when seek to 0 If seek to 0 is tried after seek to non zero position, the range header includes the previous seek position. I know this condition has been submitted by https://bugzilla.gnome.org/show_bug.cgi?id=776720 but still needs to be considered to roll back.
Review of attachment 347778 [details] [review]: Please remove the "common" change from the patch ::: ext/soup/gstsouphttpsrc.c @@ +1474,2 @@ /* Update the position if we are retrying */ + if (src->msg && src->request_position != src->read_position) { This would mean that we also add the Range header from 0 in other cases when it wasn't there in the beginning. Maybe if request_position == 0, we should just *remove* the Range header?
Created attachment 347814 [details] [review] Removed "common" from changes
@Sebastian, In the function gst_soup_http_src_add_range_header(), it only removes range headers and update the read_position so I think this simple change can make what we want. Please let me have your insight. Thanks.
Created attachment 347817 [details] [review] souphttpsrc: Remove range header when seek to 0 Remove range header when seek to 0 and update the read_position.
(In reply to Sebastian Dröge (slomo) from comment #1) > Review of attachment 347778 [details] [review] [review]: > > Please remove the "common" change from the patch > > ::: ext/soup/gstsouphttpsrc.c > @@ +1474,2 @@ > /* Update the position if we are retrying */ > + if (src->msg && src->request_position != src->read_position) { > > This would mean that we also add the Range header from 0 in other cases when > it wasn't there in the beginning. > > Maybe if request_position == 0, we should just *remove* the Range header? I uploaded a new patch "souphttpsrc: Remove range header when seek to 0" to follow your feedback. Please let me know if you feel need to change some. Thanks.
I can't reproduce the issue you're trying to fix here, how do you trigger a wrong offset in the range header ? Just reverting the patch would break retry on error. If you remove the range you will break cases where request_position is 0 but stop_position is set.
@Arnaud Vrac, You can reproduce if you try seek to some position and seek to "0". I still don`t understand "where request_position is 0 but stop_position is set." Even though in that case, we do not want to send range header from the previous seek.
Is this fixed by this commit now: https://bugzilla.gnome.org/show_bug.cgi?id=776720#c14 ?
I don`t think it is fixed by the commit https://bugzilla.gnome.org/show_bug.cgi?id=776720 . The situations are different.
This is somewhat related to the other fix and makes sense imho. It can be simplified by just removing the header (the read_position = requested_position is now done unconditionally below). Justin: Can you simplify it to just removing the range header ? Arnaud: We definitely need feedback from you to know if it doesn't break your use-case.
I pushed the commit with the simplification. commit b4edfb5998a7d9cc28e245ee4fe92720218450f1 (HEAD -> master) Author: paul.kim <paul.hyunil@lge.com> Date: Mon Mar 13 18:14:12 2017 +0900 souphttpsrc: Remove range header when seek to 0 This fixes the previous range header is remained if seek to 0 is attempted. https://bugzilla.gnome.org/show_bug.cgi?id=779957 diff --git a/ext/soup/gstsouphttpsrc.c b/ext/soup/gstsouphttpsrc.c index ff64d71c9..975692440 100644 --- a/ext/soup/gstsouphttpsrc.c +++ b/ext/soup/gstsouphttpsrc.c @@ -1580,7 +1580,8 @@ gst_soup_http_src_do_request (GstSoupHTTPSrc * src, const gchar * method) if (src->msg && src->request_position > 0) { gst_soup_http_src_add_range_header (src, src->request_position, src->stop_position); - } + } else if (src->msg && src->request_position == 0) + soup_message_headers_remove (src->msg->request_headers, "Range"); /* add_range_header() has the side effect of setting read_position to * the requested position. This *needs* to be set regardless of having
And backported to 1.12