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 779957 - souphttpsrc: Manipulate range header when seek to 0
souphttpsrc: Manipulate range header when seek to 0
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal major
: 1.12.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-12 23:06 UTC by Paul Kim
Modified: 2017-11-10 09:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
souphttpsrc: Manipulate range header when seek to 0 (1.45 KB, patch)
2017-03-12 23:06 UTC, Paul Kim
none Details | Review
Removed "common" from changes (1.18 KB, patch)
2017-03-13 09:17 UTC, Paul Kim
none Details | Review
souphttpsrc: Remove range header when seek to 0 (1.01 KB, patch)
2017-03-13 09:54 UTC, Paul Kim
none Details | Review

Description Paul Kim 2017-03-12 23:06:12 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.
Comment 1 Sebastian Dröge (slomo) 2017-03-13 08:51:19 UTC
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?
Comment 2 Paul Kim 2017-03-13 09:17:17 UTC
Created attachment 347814 [details] [review]
Removed "common" from changes
Comment 3 Paul Kim 2017-03-13 09:22:05 UTC
@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.
Comment 4 Paul Kim 2017-03-13 09:54:07 UTC
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.
Comment 5 Paul Kim 2017-03-13 09:54:55 UTC
(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.
Comment 6 Arnaud Vrac 2017-03-16 17:19:25 UTC
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.
Comment 7 Paul Kim 2017-03-16 23:09:18 UTC
@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.
Comment 8 Sebastian Dröge (slomo) 2017-11-09 23:27:09 UTC
Is this fixed by this commit now: https://bugzilla.gnome.org/show_bug.cgi?id=776720#c14 ?
Comment 9 Paul Kim 2017-11-09 23:33:34 UTC
I don`t think it is fixed by the commit https://bugzilla.gnome.org/show_bug.cgi?id=776720 . The situations are different.
Comment 10 Edward Hervey 2017-11-10 06:35:04 UTC
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.
Comment 11 Edward Hervey 2017-11-10 09:23:43 UTC
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
Comment 12 Edward Hervey 2017-11-10 09:25:51 UTC
And backported to 1.12