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.

Sign up for GitHub

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

Jump to bottom

Don't ignore InbandEventStreams at the Representation level #687

Merged
merged 1 commit into from
Feb 10, 2017
Merged

Don't ignore InbandEventStreams at the Representation level #687

merged 1 commit into from
Feb 10, 2017

Conversation

sanbornhilland
Copy link
Contributor

Closes #686

joeyparrish
joeyparrish requested changes Feb 8, 2017
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish 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 otherwise. Thanks!

lib/dash/dash_parser.js Outdated
var XmlUtils = shaka.util.XmlUtils;

var adaptationEventStream = XmlUtils.findChild(elem, 'InbandEventStream');
var representation = XmlUtils.findChild(elem, 'Representation');
Copy link
Member

Choose a reason for hiding this comment

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

This only works if there is exactly one Representation. You should probably use findChildren instead, and loop through them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot, good catch. Will update.

lib/dash/dash_parser.js Outdated
shaka.dash.DashParser.prototype.eventStreamIsPresent_ = function(elem) {
var XmlUtils = shaka.util.XmlUtils;

var adaptationEventStream = XmlUtils.findChild(elem, 'InbandEventStream');
Copy link
Member

Choose a reason for hiding this comment

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

You could return early here if adaptationEventStream

test/dash/dash_parser_manifest_unit.js
'<MPD minBufferTime="PT75S">',
' <Period id="1" duration="PT30S">',
' <AdaptationSet mimeType="video/mp4">',
' <Representation bandwidth="1">',
Copy link
Member

Choose a reason for hiding this comment

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

Create a test case with multiple Representations, please

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 rolled this into a single test because I thought it would be sufficient but I can separate the single Representation case from the multiple Representation case if you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Single test is fine. Thanks!

@sanbornhilland sanbornhilland force-pushed the bugFix/ignoredEventStreams branch from d184ac5 to 1a9b101 Compare February 8, 2017 23:09
joeyparrish
joeyparrish reviewed Feb 8, 2017
View reviewed changes
lib/dash/dash_parser.js Outdated
XmlUtils.findChild(representations[i], 'InbandEventStream');

if (representationEventStream)
break;
Copy link
Member

Choose a reason for hiding this comment

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

return true?

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 could return true there and return false at the end of the function but I was trying to avoid so many return statements so I handled it with one return at the end of the function.

joeyparrish
joeyparrish approved these changes Feb 8, 2017
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish 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. Just one nit. I think we should just return true when we find InbandEventStream inside Representation instead of breaking.

@sanbornhilland sanbornhilland force-pushed the bugFix/ignoredEventStreams branch from 1a9b101 to 1527fc6 Compare February 9, 2017 21:40
@sanbornhilland
Copy link
Contributor Author

If you guys are happy with these changes, can we merge this?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish
Copy link
Member

Looks good, thanks!

@joeyparrish joeyparrish merged commit c39e405 into shaka-project:master Feb 10, 2017
joeyparrish pushed a commit that referenced this pull request Feb 10, 2017
@sanbornhilland @joeyparrish
Don't ignore InbandEventStreams at the Representation level (#687)
8772ce0 
Backported to v2.0.x
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sanbornhilland @googlebot @shaka-bot @joeyparrish