-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 config option for using segment relative timestamps for VTT #542
Conversation
* | ||
* @throws InvalidAccessError if blank MIME types are given | ||
* @throws NotSupportedError if unsupported MIME types are given | ||
* @throws QuotaExceededError if the browser can't support that many buffers | ||
* | ||
* @suppress {unnecessaryCasts} | ||
*/ | ||
shaka.media.MediaSourceEngine.prototype.init = function(typeConfig) { | ||
shaka.media.MediaSourceEngine.prototype.init = function(typeConfig, config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just pass a useRelativeCueTimestamps boolean down instead of the whole config structure. This means we won't be able to update it, but given the nature of the setting this should be fine.
@@ -31,10 +31,12 @@ goog.require('shaka.util.Mp4Parser'); | |||
* @param {number} offset | |||
* @param {?number} segmentStartTime | |||
* @param {?number} segmentEndTime | |||
* @param {?boolean} useRelativeCueTimestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"?" in front of the type means "nullable". E. g. segmentEndTime could be a number, or it could be null. Notice, how the offset is just {number}, meaning we always expect it to be a number, passing null won't compile.
In case of useRelativeCueTimestamps, it always should be either true or false, which means we don't need the "?" before "boolean". Same stands for all the other places where it is declared.
@@ -45,7 +47,7 @@ shaka.media.Mp4TtmlParser = | |||
// mdat box found, use TtmlTextParser to parse the content | |||
return shaka.media.TtmlTextParser( | |||
reader.readBytes(boxSize - 8).buffer, offset, | |||
segmentStartTime, segmentEndTime); | |||
segmentStartTime, segmentEndTime, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's less confusing if you pass the value of useRelativeCueTimestamps, the other parsers can ignore it.
@@ -34,10 +34,12 @@ goog.require('shaka.util.TextParser'); | |||
* @param {number} offset | |||
* @param {?number} segmentStartTime | |||
* @param {?number} segmentEndTime | |||
* @param {?boolean} useRelativeCueTimestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "?" and add a comment that useRelativeCueTimestamps is only used by the VTT parser.
* @implements {shaka.util.IDestroyable} | ||
*/ | ||
shaka.media.TextEngine = function(track, mimeType) { | ||
shaka.media.TextEngine = function(track, mimeType, config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use bool useRelativeCueTimestamps
@@ -30,11 +30,14 @@ goog.require('shaka.util.StringUtils'); | |||
* @param {number} offset | |||
* @param {?number} segmentStartTime | |||
* @param {?number} segmentEndTime | |||
* @param {?boolean} useRelativeCueTimestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop "?" and add a comment that useRelativeCueTimestamps is only used by the VTT parser.
@@ -49,7 +52,7 @@ shaka.media.TtmlTextParser = | |||
} | |||
|
|||
if (xml) { | |||
// Try to get the framerate, subRameRate and frameRateMultiplier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
@@ -65,6 +65,6 @@ describe('Cue', function() { | |||
*/ | |||
function parseVtt(text, opt_offset) { | |||
var data = shaka.util.StringUtils.toUTF8(text); | |||
return shaka.media.VttTextParser(data, opt_offset || 0, null, null); | |||
return shaka.media.VttTextParser(data, opt_offset || 0, null, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use false, here and in the other tests
@@ -368,7 +368,8 @@ describe('VttTextParser', function() { | |||
function verifyHelper(cues, text, opt_offset) { | |||
var data = shaka.util.StringUtils.toUTF8(text); | |||
// Last two parameters are only used by mp4 vtt parser. | |||
var result = shaka.media.VttTextParser(data, opt_offset || 0, null, null); | |||
var result = | |||
shaka.media.VttTextParser(data, opt_offset || 0, null, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a new test "uses relative time stamps if configured to" which will pass "true" to the vtt parser and check that the start/end time of the cues is now relative.
Testing in progress... |
Failure:
|
It might be better to log the warning specifically from the VTTParser, and specifically when the start offset is non-zero. That way, if the VTT parser is not being used, or if the WebVTT tracks are not segmented, the warning will not be seen. This should also fix the test failures. |
Okay, thanks, will do. I was confused about those test failures. Thanks for the explanation. |
The CL looks good otherwise :) |
@@ -365,7 +365,7 @@ describe('VttTextParser', function() { | |||
}); | |||
|
|||
it('ignores and logs invalid settings', function() { | |||
expect(logWarningSpy.calls.count()).toBe(0); | |||
expect(logWarningSpy.calls.count()).toBe(1); // From test above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this test is run in isolation, it would still fail. The right fix is to add a beforeEach() function, in which you call logWarningSpy.calls.reset(). That way, each test gets a clean slate. This is something that we should caught long ago.
@@ -39,6 +39,12 @@ shaka.media.VttTextParser = | |||
function(data, offset, segmentStartTime, | |||
segmentEndTime, useRelativeCueTimestamps) { | |||
|
|||
if (segmentStartTime > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& !useRelativeCueTimestamps ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unsure about this and wanted feedback. I thought it might be desirable for all users to get this warning since deprecating this will affect users whether they are using the default setting or not. But if that seems like overkill than I can put that second condition back in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the flag is set to true, then somebody is using it explicitly to get the new behavior, so they don't really need a warning. At least, that's my take on it.
Testing in progress... |
All tests passed! |
* Add config option for using segment relative timestamps for VTT Fix for #480 * Make useRelativeCueTimestamps a non-nullable param * Update tests for the new useRelativeCueTimestamps param * Move period relative timestamp deprecation warning to vtt parser * Log warning only if using absolute timestamps in text cue * Fix vtt text parser test
Fix for #480
This should be reviewed closely. Specifically I'm not thrilled that I had to modify all the other text parsers to receiver an argument that none of them are going to use. That seems confusing to me but I wasn't sure how to avoid that.