-
Notifications
You must be signed in to change notification settings - Fork 113
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
Filmstrip interval should be determined from rep metadata field #23
Conversation
Was trying to add tests for this in MediaControls-test.js's filmstripShowHandler when I found this comment left by Jeremy: "unable to test case where the filmstrip placeholder is not used" I need to sync up with Jeremy/team to understand why I shouldn't be testing that case. |
src/lib/viewers/media/Dash.js
Outdated
@@ -319,8 +319,9 @@ class Dash extends VideoBase { | |||
const filmstrip = getRepresentation(this.options.file, 'filmstrip'); | |||
if (filmstrip) { | |||
const url = this.createContentUrlWithAuthParams(filmstrip.content.url_template); | |||
const interval = filmstrip.metadata ? filmstrip.metadata.interval : 1; |
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.
// The filmstrip has one frame every 'interval' seconds
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.
done
src/lib/viewers/media/Dash.js
Outdated
@@ -319,8 +319,10 @@ class Dash extends VideoBase { | |||
const filmstrip = getRepresentation(this.options.file, 'filmstrip'); | |||
if (filmstrip) { | |||
const url = this.createContentUrlWithAuthParams(filmstrip.content.url_template); | |||
// The filmstrip has one frame every 'interval' seconds | |||
const interval = filmstrip.metadata ? filmstrip.metadata.interval : 1; |
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.
might want to check for filmstrip.metadata && filmstrip.metadata.interval to be safe.
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.
const interval = filmstrip.metadata ? filmstrip.metadata.interval || 1 : 1;
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 think if interval isn't there, we cannot just assume that it's 1s. It might be because of some kind of data-corruption on the backend the day we do dynamic intervals. If interval isn't there, the playback still works so I'm fine with leaving this as is.
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.
Does that also apply if metadata isn't there?
Also, if we want the behavior to be not showing filmstrips if interval isn't present, we should explicitly code for that instead of relying on (time / undefined)
to fail in filmstripShowHandler()
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.
good point. Assuming metadata/interval will always be there instead. Relies on my php commit to go out first though (if there's a delay otherwise, filmstrips on the newest preview lib will not show until the php code makes it out, but that's probably fine since it's temporary).
After talking with Jeremy, I think I'll follow up this commit with a refactor of filmstripShowHandler to pull out the computation into a more testable function somehow. |
* @return {void} | ||
*/ | ||
initFilmstrip(url, status, aspect) { | ||
initFilmstrip(url, status, aspect, interval) { |
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.
Could you give interval
a default value of 1?
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 don't want to default it because the default value of 1 might not be correct (see my comment above)
@@ -317,10 +317,10 @@ class Dash extends VideoBase { | |||
*/ | |||
loadFilmStrip() { | |||
const filmstrip = getRepresentation(this.options.file, 'filmstrip'); | |||
if (filmstrip) { | |||
if (filmstrip && filmstrip.metadata && filmstrip.metadata.interval > 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.
// Do not load filmstrip if the interval metadata property isn't set
Before, the filmstrip handler assumed that each frame in the filmstrip corresponded to a single second of the video. Now, it reads a metadata field from the API to determine the duration of each frame.
They will exist after https://scm.dev.box.net/#/c/373490/ goes live In general, if they don't exist, you can't assume any particular interval. It's better to not load the filmstrip at all in these cases.
https://github.com/box/box-annotations/releases # 0.3.0 (2017-11-14) * Chore: Add conventional-changelog-releaser (box#34) ([624e0bd](box/box-annotations@624e0bd)) * Chore: Do not clear out node_modules on when publishing npm package (box#30) ([08c180d](box/box-annotations@08c180d)) * Fix: Do not select drawings when creating a point annotation on top (box#28) ([07fc4c1](box/box-annotations@07fc4c1)) * Fix: Drawing CSS (box#23) ([0493fcd](box/box-annotations@0493fcd)) * Fix: fix highlight selection and typos (box#21) ([ca54d5e](box/box-annotations@ca54d5e)), closes [box#21](box/box-annotations#21) * Fix: Hide createHighlightDialog on page re-render (box#31) ([0b74717](box/box-annotations@0b74717)) * Fix: Match width of reply textarea with comments (box#33) ([5f0f29b](box/box-annotations@5f0f29b)) * Fix: Only registering thread with controller on save (box#22) ([ff75bf1](box/box-annotations@ff75bf1)) * Fix: Only show create dialog when creating new point annotations (box#29) ([4822ece](box/box-annotations@4822ece)) * Fix: only show newly created annotation dialog on hover (box#25) ([0ba1965](box/box-annotations@0ba1965)) * Fix: Position create dialog properly near page edges (box#32) ([968efb3](box/box-annotations@968efb3)) * Fix: release script should use correct remote branch (box#20) ([8bd12a5](box/box-annotations@8bd12a5)) * Fix: Scrolling on highlight dialogs in powerpoints (box#24) ([b21ed0e](box/box-annotations@b21ed0e)) * Fix: Validation message fails to get displayed when user clicks on 'Post' without entering any comme ([d90e72c](box/box-annotations@d90e72c)) * Fix: Hide mobile dialog on first outside click (box#26) ([01ec48b](box/box-annotations@01ec48b))
https://github.com/box/box-annotations/releases # 0.3.0 (2017-11-14) * Chore: Add conventional-changelog-releaser (#34) ([624e0bd](box/box-annotations@624e0bd)) * Chore: Do not clear out node_modules on when publishing npm package (#30) ([08c180d](box/box-annotations@08c180d)) * Fix: Do not select drawings when creating a point annotation on top (#28) ([07fc4c1](box/box-annotations@07fc4c1)) * Fix: Drawing CSS (#23) ([0493fcd](box/box-annotations@0493fcd)) * Fix: fix highlight selection and typos (#21) ([ca54d5e](box/box-annotations@ca54d5e)), closes [#21](box/box-annotations#21) * Fix: Hide createHighlightDialog on page re-render (#31) ([0b74717](box/box-annotations@0b74717)) * Fix: Match width of reply textarea with comments (#33) ([5f0f29b](box/box-annotations@5f0f29b)) * Fix: Only registering thread with controller on save (#22) ([ff75bf1](box/box-annotations@ff75bf1)) * Fix: Only show create dialog when creating new point annotations (#29) ([4822ece](box/box-annotations@4822ece)) * Fix: only show newly created annotation dialog on hover (#25) ([0ba1965](box/box-annotations@0ba1965)) * Fix: Position create dialog properly near page edges (#32) ([968efb3](box/box-annotations@968efb3)) * Fix: release script should use correct remote branch (#20) ([8bd12a5](box/box-annotations@8bd12a5)) * Fix: Scrolling on highlight dialogs in powerpoints (#24) ([b21ed0e](box/box-annotations@b21ed0e)) * Fix: Validation message fails to get displayed when user clicks on 'Post' without entering any comme ([d90e72c](box/box-annotations@d90e72c)) * Fix: Hide mobile dialog on first outside click (#26) ([01ec48b](box/box-annotations@01ec48b))
https://github.com/box/box-annotations/releases # 0.3.0 (2017-11-14) * Chore: Add conventional-changelog-releaser (box#34) ([624e0bd](box/box-annotations@624e0bd)) * Chore: Do not clear out node_modules on when publishing npm package (box#30) ([08c180d](box/box-annotations@08c180d)) * Fix: Do not select drawings when creating a point annotation on top (box#28) ([07fc4c1](box/box-annotations@07fc4c1)) * Fix: Drawing CSS (box#23) ([0493fcd](box/box-annotations@0493fcd)) * Fix: fix highlight selection and typos (box#21) ([ca54d5e](box/box-annotations@ca54d5e)), closes [box#21](box/box-annotations#21) * Fix: Hide createHighlightDialog on page re-render (box#31) ([0b74717](box/box-annotations@0b74717)) * Fix: Match width of reply textarea with comments (box#33) ([5f0f29b](box/box-annotations@5f0f29b)) * Fix: Only registering thread with controller on save (box#22) ([ff75bf1](box/box-annotations@ff75bf1)) * Fix: Only show create dialog when creating new point annotations (box#29) ([4822ece](box/box-annotations@4822ece)) * Fix: only show newly created annotation dialog on hover (box#25) ([0ba1965](box/box-annotations@0ba1965)) * Fix: Position create dialog properly near page edges (box#32) ([968efb3](box/box-annotations@968efb3)) * Fix: release script should use correct remote branch (box#20) ([8bd12a5](box/box-annotations@8bd12a5)) * Fix: Scrolling on highlight dialogs in powerpoints (box#24) ([b21ed0e](box/box-annotations@b21ed0e)) * Fix: Validation message fails to get displayed when user clicks on 'Post' without entering any comme ([d90e72c](box/box-annotations@d90e72c)) * Fix: Hide mobile dialog on first outside click (box#26) ([01ec48b](box/box-annotations@01ec48b))
Before, the filmstrip handler assumed that each frame in the filmstrip
corresponded to a single second of the video. Now, it reads a metadata
field from the API to determine the duration of each frame.