Add support for 302 redirect #266

rgc · 2016-01-12T19:14:21Z

Add support for responseURL where supported.

XMLHttpRequest spec 4.6.1 adds the responseUrl attribute:
https://xhr.spec.whatwg.org/#the-responseurl-attribute

Browser Support
Chromium: https://code.google.com/p/chromium/issues/detail?id=377583
Webkit: https://bugs.webkit.org/show_bug.cgi?id=136938
Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=998076

A current use case is when the DASH manifest is located at a URL that is redirected via 302 to another location. Currently, the mpdParser will then pull the chunks from the original URI rather than the URl from which the manifest was ultimately downloaded.

Example - Current Flow
http://some.host.org/path/manifest.mpd returns 302 to http://other.host.org/path/manifest.mpd.

Subsequent requests for mp4 chunks will be:
http://some.host.org/path/chunk1.mp4
http://some.host.org/path/chunk2.mp4

If the server has been configured to 302 redirect for everything, this will result in each of the above chunks also having a 302 redirect. If the server hasn't been configured, then they will fail.

Example - Proposed Flow
http://some.host.org/path/manifest.mpd returns 302 to http://other.host.org/path/manifest.mpd.

Subsequent requests for mp4 chunks will be:
http://other.host.org/path/chunk1.mp4
http://other.host.org/path/chunk2.mp4

As the base_url should be the URI from which the manifest was downloaded.

In browsers without responseUrl support, flow will fallback to current.

joeyparrish · 2016-01-12T19:16:44Z

lib/util/failover_uri.js

@@ -168,6 +168,9 @@ shaka.util.FailoverUri.prototype.createRequest_ =
        // effectively cache every response.
        this.requestPromise_ = null;
        this.request_ = null;
+        if (xhr.responseURL) {
+          this.urls.unshift(new goog.Uri(xhr.responseURL));


What would be better (IMO) than prepending the URL would be to replace the specific URL this request used. (this.urls[i])

rgc · 2016-01-12T19:16:54Z

Agree - replacing (this.urls[i]) is better - will do that.

rgc · 2016-01-12T19:28:24Z

@joeyparrish - Lint/Build passed and I've implemented the replace rather than the unshift. Will read the contrib before committing next time ;)

Thanks!

joeyparrish · 2016-01-12T19:29:15Z

GitHub is saying you have merge conflicts. Please rebase to the tip of master and manually resolve the merge conflicts. Thanks!

rgc · 2016-01-12T20:05:21Z

Conflicts have been resolved -- Thanks

shaka-bot · 2016-01-12T20:32:47Z

Testing in progress...

joeyparrish · 2016-01-12T20:42:34Z

We are experiencing some flakiness on our buildbot. Please forgive the noise.

rgc · 2016-01-12T20:44:45Z

I think it is failing b/c the URLs are formatted in the tests as:
//some.host.org/path/dash.mpd
and responseUrl will typically contain the proto and params:
http://some.host.org/path/dash.mpd?m=1212121

So the test is failing as it notices they don't match...

I'll look into it.

joeyparrish · 2016-01-12T20:48:05Z

Yes, that seems to be the case. Not flake at all.

Here's the log snippet:

Chrome 47.0.2526 (Linux 0.0.0) Live live support for segment time template requests MPD update in expected time FAILED
    Expected 'http://storage.googleapis.com/widevine-demo-media/oops-segment-timeline-time/oops-segment-timeline-time-oops_video.mpd?_=1452630896703' to equal '//storage.googleapis.com/widevine-demo-media/oops-segment-timeline-time/oops-segment-timeline-time-oops_video.mpd'.

rgc · 2016-01-12T22:14:47Z

I'm pondering the best way to fix.

It looks like we have an expectation that the final MPD location should match the target location:

spyOn(window.shaka.dash, 'MpdRequest').and.callFake(function(mpdUrl) {
      expect(mpdUrl.toString()).toEqual(targetMpdUrl);

I could strip the proto and params and get it to pass the specific test case, but that's not right.

Ultimately, this expectation isn't going to pass ever in cases where the player was redirected, so I'll need to come up with a better test to fit.

joeyparrish · 2016-01-12T22:56:40Z

Options:

Hack XMLHttpRequest to suppress responseURL during the test, then restore it afterward. delete XMLHttpRequest.prototype['responseURL']
Decide it's wrong to expect a specific URL, since this is not relevant to the test. Remove the targetMpdUrl parameter from waitForMpdRequest().

My preference is for 2.

rgc · 2016-01-13T18:31:07Z

I was testing my solution last night and noticed that by replacing urls[i], we doom future manifest requests. The proper functionality would be for the relative chunks detailed in the manifest to use the 302 location for the relative baseUrl, however future manifest requests should not.

Below is an updated version of flow which encompasses this detail.

Example - Proposed Flow
http://some.host.org/path/manifest.mpd --> 302 --> http://other.host.org/path/manifest.mpd.
http://other.host.org/path/chunk1.mp4
http://other.host.org/path/chunk2.mp4
(manifest update)
http://some.host.org/path/manifest.mpd --> 302 --> http://another.host.org/path/manifest.mpd.
http://another.host.org/path/chunk1.mp4
http://another.host.org/path/chunk2.mp4

I've committed a few more changes that fix the flow. I have not altered the failing test, as I believe these new changes may not trigger it.

joeyparrish · 2016-01-13T18:47:19Z

lib/util/failover_uri.js

+
+  /** @public {goog.Uri} */
+  this.currentUrl = null;
+


Drop this extra line

joeyparrish · 2016-01-13T18:54:34Z

One last request: can you squash these commits together? It's kind of hard to parse the log with 10 commits, 7 of which have the same message.

…ents for XHR

…URL that is seperate from original URL

rgc · 2016-01-13T20:26:50Z

@joeyparrish - squashed.

Thanks much!

shaka-bot · 2016-01-13T21:18:09Z

Testing in progress...

shaka-bot · 2016-01-13T21:23:13Z

Failure:

Chrome 47.0.2526 (Linux 0.0.0) mpd.BaseUrl resolves relative URLs with respect to the MPD URL FAILED
    TypeError: Cannot read property 'toString' of undefined
        at Object.<anonymous> (/var/lib/jenkins/jobs/Manual PR Test/workspace/spec/mpd_parser_base_url_spec.js:152:23)
Chrome 47.0.2526 (Linux 0.0.0) mpd handles a Location element with a BaseURL FAILED
    Expected 'updated_mpd' to be 'http://example.com/updated_mpd'.
        at Object.<anonymous> (/var/lib/jenkins/jobs/Manual PR Test/workspace/spec/mpd_spec.js:555:43)

joeyparrish · 2016-01-13T22:48:15Z

@rgc Still getting failures. I've edited the buildbot message in this thread to just the failures. (In the mean time, I'm working on getting our buildbot to send the failures only and to format the message correctly. Should be easier to read if we see failures next time.)

rgc · 2016-01-14T16:18:57Z

@joeyparrish - I was able to run the tests locally, find the issue and fix it.

rgc · 2016-01-14T20:49:33Z

I fixed the spec tests to pass the proper number of arguments to shaka.dash.mpd.parseMpd as well

joeyparrish · 2016-01-14T21:08:42Z

lib/dash/mpd_parser.js

@@ -50,8 +51,12 @@ shaka.dash.mpd.parseMpd = function(source, url) {
    return null;
  }

+  if (!baseUrl) {


You should drop this condition, since it should not be possible after successful compilation. Otherwise, the PR looks good to me.

rgc · 2016-01-14T21:16:23Z

@joeyparrish - agreed, done and squashed.

joeyparrish · 2016-01-14T21:18:03Z

Looks good! I'll give the buildbot one more crack at it to verify. Thanks for taking the time to do revisions and get everything done cleanly.

shaka-bot · 2016-01-14T21:18:31Z

Testing in progress...

shaka-bot · 2016-01-14T21:23:49Z

All tests passed!

Add support for 302 redirect

joeyparrish · 2016-01-14T21:24:56Z

Thanks very much for the contribution. We will implement something equivalent in Shaka 2 soon to make sure this feature isn't lost in the rewrite.

rgc · 2016-01-14T21:25:47Z

Thanks for walking me through the process -- Cheers!

When possible, use xhr.responseURL to detect 302 redirects and make the information available in the Response object. Use redirect URI as BaseURL for manifest parsing. See also #225, #266 Change-Id: Ie24abeb3b8418b3e89fed6666eb525aecd74f03b

joeyparrish mentioned this pull request Jan 12, 2016

302 redirect detection #265

Closed

joeyparrish reviewed Jan 12, 2016
View reviewed changes

rgc mentioned this pull request Jan 12, 2016

Standard HTTP Redirect #225

Closed

joeyparrish self-assigned this Jan 12, 2016

joeyparrish added the type: enhancement New feature or request label Jan 12, 2016

joeyparrish reviewed Jan 13, 2016
View reviewed changes

lib/util/failover_uri.js

/** @public {goog.Uri} */

this.currentUrl = null;

Copy link

Member

joeyparrish Jan 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this extra line

Rob Colantuoni added 3 commits January 13, 2016 15:11

Add support for 302 redirect in browsers that have responseURL implem…

e20cff5

…ents for XHR

Adding myself to Authors/Contrib following CLA submission

f512c52

Update that will allow for 302 redirect on manifest to be passed Base…

0122c11

…URL that is seperate from original URL

joeyparrish reviewed Jan 14, 2016
View reviewed changes

Fix tests to pass two urls to parseMpd

471a950

joeyparrish added a commit that referenced this pull request Jan 14, 2016

Merge pull request #266 from rgc/redirect-fix

f21a725

Add support for 302 redirect

joeyparrish merged commit f21a725 into shaka-project:master Jan 14, 2016

rgc mentioned this pull request Nov 15, 2016

If manifest returns 301/302, build the segment list relative to the redirect videojs/videojs-contrib-hls#912

Closed

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for 302 redirect #266

Add support for 302 redirect #266

rgc commented Jan 12, 2016

joeyparrish Jan 12, 2016

rgc commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

shaka-bot commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 13, 2016

joeyparrish Jan 13, 2016

joeyparrish commented Jan 13, 2016

rgc commented Jan 13, 2016

shaka-bot commented Jan 13, 2016

shaka-bot commented Jan 13, 2016

joeyparrish commented Jan 13, 2016

rgc commented Jan 14, 2016

rgc commented Jan 14, 2016

joeyparrish Jan 14, 2016

rgc commented Jan 14, 2016

joeyparrish commented Jan 14, 2016

shaka-bot commented Jan 14, 2016

shaka-bot commented Jan 14, 2016

joeyparrish commented Jan 14, 2016

rgc commented Jan 14, 2016

Add support for 302 redirect #266

Add support for 302 redirect #266

Conversation

rgc commented Jan 12, 2016

joeyparrish Jan 12, 2016

Choose a reason for hiding this comment

rgc commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

shaka-bot commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 12, 2016

joeyparrish commented Jan 12, 2016

rgc commented Jan 13, 2016

joeyparrish Jan 13, 2016

Choose a reason for hiding this comment

joeyparrish commented Jan 13, 2016

rgc commented Jan 13, 2016

shaka-bot commented Jan 13, 2016

shaka-bot commented Jan 13, 2016

joeyparrish commented Jan 13, 2016

rgc commented Jan 14, 2016

rgc commented Jan 14, 2016

joeyparrish Jan 14, 2016

Choose a reason for hiding this comment

rgc commented Jan 14, 2016

joeyparrish commented Jan 14, 2016

shaka-bot commented Jan 14, 2016

shaka-bot commented Jan 14, 2016

joeyparrish commented Jan 14, 2016

rgc commented Jan 14, 2016