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

Add subtitles, video, and audio group-id refs to StreamInfo #126

Merged
merged 2 commits into from
Oct 13, 2018

Conversation

cdunklau
Copy link
Contributor

This partially addresses #121

I'm not sure about this... it seems like this could be a potentially breaking change, and I'm suspicious about the tests all passing with it. It seems to me that this (and #125) should have broken playlist serialization.

Please review.

@coveralls
Copy link

coveralls commented Oct 12, 2018

Coverage Status

Coverage remained the same at 97.095% when pulling fb6ef2a on cdunklau:expand-streaminfo into 12550e3 on globocom:master.

@leandromoreira
Copy link
Contributor

Hey @cdunklau thanks for the contribution, it is amazing such effort! In regard to your worries, I think this PR doesn't offer any breaking change, it adds features, therefore, all the tests should be passing without any issues.

#EXT-X-STREAM-INF:PROGRAM-ID=1,BANDWIDTH=65000,CLOSED-CAPTIONS="cc",SUBTITLES="sub",AUDIO="aud",VIDEO="vid"
http://example.com/with-everything-low.m3u8
'''

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use fake uri (i.e: http://example.com/cc.srt) instead of `"cc".

CLOSED-CAPTION does not specify a Rendition; the closed caption
      media is present in the Media Segments of every video Rendition.

      URI

      The value is a quoted-string containing a URI that identifies the
      Media Playlist file.  This attribute is OPTIONAL; see
      Section 4.3.4.2.1.  If the TYPE is CLOSED-CAPTIONS, the URI
      attribute MUST NOT be present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're conflating the URI attribute of EXT-X-I-FRAME-STREAM-INF with the URI line that must follow EXT-X-STREAM-INF. In EXT-X-STREAM-INF, the CLOSED-CAPTIONS, AUDIO, VIDEO, and SUBTITLES attributes all reference a "GROUP-ID attribute of an EXT-X-MEDIA tag elsewhere".

However, reviewing this made me realize that StreamInfo for EXT-X-I-FRAME-STREAM-INF should have audio, subtitles, and closed-captions set to None explicitly.

All attributes defined for the EXT-X-STREAM-INF tag (Section 4.3.4.2)
are also defined for the EXT-X-I-FRAME-STREAM-INF tag, except for the
FRAME-RATE, AUDIO, SUBTITLES, and CLOSED-CAPTIONS attributes.

@leandromoreira
Copy link
Contributor

Do you plan to do any other PR soon? I'm thinking to release a new version.

@cdunklau
Copy link
Contributor Author

@leandromoreira No concrete plans, no. I'll probably take another look at #121 next week, but it seems like the other issue there will take some more digging.

@leandromoreira leandromoreira merged commit e499d3c into globocom:master Oct 13, 2018
@cdunklau cdunklau deleted the expand-streaminfo branch October 13, 2018 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants