Skip to content

streams.dash: Support manifest strings in addition to manifest urls #2285

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

Merged
merged 3 commits into from
Feb 13, 2019

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Feb 1, 2019

Modifies parse_manifest() to accept a url_or_manifest parameter.

See also: #2239

This does not include any of the changes for supporting DASH SegmentBase.

Addresses streamlink#2179. Allows parse_manifest() to accept a string
containing a properly-formatted XML manifest instead of a
manifest url.
@codecov
Copy link

codecov bot commented Feb 1, 2019

Codecov Report

Merging #2285 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
- Coverage   52.61%   52.57%   -0.05%     
==========================================
  Files         237      237              
  Lines       14758    14815      +57     
==========================================
+ Hits         7765     7789      +24     
- Misses       6993     7026      +33

Copy link
Collaborator

@back-to back-to left a comment

Choose a reason for hiding this comment

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

if url_or_manifest is used and there is drm protection in the string

raise PluginError("{} is protected by DRM".format(url))

there will be an NameError: name 'url' is not defined

maybe not that important as this is not supported anyways

@back-to back-to assigned back-to and unassigned back-to Feb 9, 2019
@back-to back-to requested a review from beardypig February 9, 2019 17:30
Copy link
Member

@beardypig beardypig left a comment

Choose a reason for hiding this comment

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

Looks good, but I think the test could cover more :)

@@ -208,6 +208,26 @@ def test_parse_manifest_drm(self, mpdClass):
self.session, self.test_url)
mpdClass.assert_called_with(ANY, base_url="http://test.bar", url="http://test.bar/foo.mpd")

@patch('streamlink.stream.dash.MPD')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this test is quite valid, it doesn't test that the correct thing is passed to the MPD class. I think parsing one of the included resources might be a better test.

    def test_parse_manifest_string(self):
        with text("dash/test_9.mpd") as mpd_txt:
            test_manifest = mpd_txt.read()

        streams = DASHStream.parse_manifest(self.session, test_manifest)

        self.assertSequenceEqual(
            sorted(list(streams.keys())),
            sorted(["2500k"])
        )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6aea0f4

@gravyboat
Copy link
Member

Thanks @pmrowla, and thanks for the reviews @back-to and @beardypig!

@gravyboat gravyboat merged commit 93174bd into streamlink:master Feb 13, 2019
jackyzy823 pushed a commit to jackyzy823/streamlink that referenced this pull request Mar 20, 2019
…treamlink#2285)

* stream.dash: Support manifest strings in addition to manifest urls

Addresses streamlink#2179. Allows parse_manifest() to accept a string
containing a properly-formatted XML manifest instead of a
manifest url.

* streams.dash: add unit test for manifest string

* streams.dash: fix parse_manifest_string unit test
Billy2011 pushed a commit to Billy2011/streamlink-27 that referenced this pull request May 14, 2020
…treamlink#2285)

* stream.dash: Support manifest strings in addition to manifest urls

Addresses streamlink#2179. Allows parse_manifest() to accept a string
containing a properly-formatted XML manifest instead of a
manifest url.

* streams.dash: add unit test for manifest string

* streams.dash: fix parse_manifest_string unit test
mkbloke pushed a commit to mkbloke/streamlink that referenced this pull request Aug 18, 2020
…treamlink#2285)

* stream.dash: Support manifest strings in addition to manifest urls

Addresses streamlink#2179. Allows parse_manifest() to accept a string
containing a properly-formatted XML manifest instead of a
manifest url.

* streams.dash: add unit test for manifest string

* streams.dash: fix parse_manifest_string unit test
resiproxy pushed a commit to resiproxy/streamlink that referenced this pull request Nov 5, 2020
…treamlink#2285)

* stream.dash: Support manifest strings in addition to manifest urls

Addresses streamlink#2179. Allows parse_manifest() to accept a string
containing a properly-formatted XML manifest instead of a
manifest url.

* streams.dash: add unit test for manifest string

* streams.dash: fix parse_manifest_string unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants