Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLS: Incorrect composition of full URLs to media_N.m3u8 #1664

Closed
Lastique opened this issue Nov 14, 2018 · 6 comments
Closed

HLS: Incorrect composition of full URLs to media_N.m3u8 #1664

Lastique opened this issue Nov 14, 2018 · 6 comments
Assignees
Labels
component: HLS The issue involves Apple's HLS manifest format status: archived Archived and locked; will not be updated type: bug Something isn't working correctly type: code health A code health issue
Milestone

Comments

@Lastique
Copy link

Have you read the FAQ and checked for duplicate open issues?:
Yes.

What version of Shaka Player are you using?:
Reproduces on 2.5.0-beta2 and 2.4.5.

Can you reproduce the issue with our latest release version?:
Yes.

Can you reproduce the issue with the latest code from master?:
I didn't try.

Are you using the demo app or your own custom app?:
Custom app.

If custom app, can you reproduce the issue using our demo app?:
I can't try as the stream is not accessible publicly.

What browser and OS are you using?:
Kubuntu 18.10, Chrome 70.0.3538.102, Firefox 63.0.

What are the manifest and license server URIs?:
No license server. The manifest and the test page are attached.

What did you do?
Suppose, the test page is located at https://my.server.com/dash/index-shaka.html. The page contains a relative path to the manifest "media/master.m3u8". The media manifests are also located in the "media" subdirectory, i.e. "media/media_0.m3u8", "media/media_1.m3u8", etc. master.m3u8 contains only media manifest file names with no path, i.e. media_0.m3u8, media_1.m3u8, etc.

What actually happened?
When I open the test page, Shaka-Player attempts to load media manifests at https://my.server.com/dash/media/media/media_0.m3u8. Notice the duplicate "media" in the URL. As a result, media manifests are not found, the server returns 404.

What did you expect to happen?
The player should correctly compose the absolute URLs to the media manifests. Given that the page is located at https://my.server.com/dash/index-shaka.html, the absolute URL to the master manifest would be https://my.server.com/dash/media/master.m3u8. Since media manifests are referenced relative to the master manifest, the correct absolute URLs should be https://my.server.com/dash/media/media_0.m3u8, etc.

I've attached the test page and manifests in the directory layout described above.
dash.tar.gz

@joeyparrish joeyparrish self-assigned this Nov 16, 2018
@joeyparrish joeyparrish added type: bug Something isn't working correctly and removed needs triage labels Nov 16, 2018
@joeyparrish
Copy link
Member

Confirmed. Thanks for the report! We're working on it.

@shaka-bot shaka-bot added this to the v2.5 milestone Nov 16, 2018
@joeyparrish
Copy link
Member

I have a working fix locally, but I still need to clean things up, update the tests, and write new tests.

@joeyparrish joeyparrish added the component: HLS The issue involves Apple's HLS manifest format label Nov 19, 2018
@joeyparrish
Copy link
Member

After a great deal of refactor and clean up (which I think we should keep), I have discovered that this bug can only be triggered if the URI loaded for the master playlist is a relative path:

player.load('media/master.m3u8');

The fix for that could be a single line in the HLS parser:

shaka.hls.HlsParser.prototype.start = function(uri, playerInterface) {
  goog.asserts.assert(this.config_, 'Must call configure() before start()!');
  this.playerInterface_ = playerInterface;
  this.manifestUri_ = uri;
  return this.requestManifest_(uri).then(function(response) {

    // vvvvv
    this.manifestUri_ = response.uri;  // ADD THIS LINE
    // ^^^^^

    return this.parseManifest_(response.data, uri).then(function() {
      this.setUpdateTimer_(this.updatePeriod_);
      return this.manifest_;
    }.bind(this));
  }.bind(this));
};

In your case, uri is a relative URI, but response.uri is absolute. The trouble comes later when we try to resolve other relative URIs against this.manifestUri_. The URI resolution code expects an absolute URI as a basis, so the results were broken and caused the duplication in your media playlist URI.

Even though I now know that we could "fix" it with that one line, there are many unclear assumptions about relative vs absolute URIs in the HLS parser, and some TODOs about how we handle redirects (poorly). So I think the larger cleanup is worthwhile.

@joeyparrish joeyparrish added the type: code health A code health issue label Nov 19, 2018
shaka-bot pushed a commit that referenced this issue Nov 19, 2018
Also document the usage of each Set/Map to make changes easier to
understand.

Issue #1664

Change-Id: I4e951c642bca13b7b3ead9b478bf530cfadbabf3
@Lastique
Copy link
Author

Thanks for the fix!

@joeyparrish
Copy link
Member

Happy to help!

joeyparrish added a commit that referenced this issue Jan 17, 2019
Also document the usage of each Set/Map to make changes easier to
understand.

Issue #1664

Backported to v2.4.x

Change-Id: I4e951c642bca13b7b3ead9b478bf530cfadbabf3
joeyparrish added a commit that referenced this issue Jan 17, 2019
This fixes issues with the interpretation of URIs in HLS and makes
their usage and meaning consistent and clear.

 - Name URI variables as either absolute, final (post-redirect), or
   verbatim (exactly as they appear in the playlist)
 - Identify media playlists by their verbatim URI when testing for
   equality or duplication
 - When a master playlist is redirected, interpret media playlists as
   relative to the redirected location
 - When a media playlist is redirected, request updates from the
   redirected location
 - When updating a media playlist, resolve media segment URIs as
   relative to the latest redirected media playlist URI
 - Resolve absolute segment URIs when parsing the playlist text,
   rather than waiting until we intepret and build the manifest
 - Remove some incidental bind() calls, which exposed compiler errors
 - Avoid refactoring long parameter lists
 - Avoid refactoring for async/await
 - Clean up redirection tests, which were brittle and did not verify
   what they seemed to
 - Use relative segment URIs for all segments in tests
 - Use media playlist URIs within master playlists in tests
 - Add a regression test specifically for #1664

Closes #1664

Backported to v2.4.x

Change-Id: I45f946790c7d669637c231ae93920a09c18c4222
@shaka-project shaka-project locked and limited conversation to collaborators Jan 18, 2019
@joeyparrish
Copy link
Member

Fix cherry-picked for v2.4.6.

@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: HLS The issue involves Apple's HLS manifest format status: archived Archived and locked; will not be updated type: bug Something isn't working correctly type: code health A code health issue
Projects
None yet
Development

No branches or pull requests

3 participants