Filmstrip interval should be determined from rep metadata field #23

bhh1988 · 2017-03-28T05:39:19Z

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.

bhh1988 · 2017-03-28T05:41:46Z

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.

tonyjin · 2017-03-28T06:15:02Z

src/lib/viewers/media/Dash.js

@@ -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;


// The filmstrip has one frame every 'interval' seconds

jeremypress · 2017-03-28T18:09:15Z

src/lib/viewers/media/Dash.js

@@ -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;


might want to check for filmstrip.metadata && filmstrip.metadata.interval to be safe.

const interval = filmstrip.metadata ? filmstrip.metadata.interval || 1 : 1;

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.

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()

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).

bhh1988 · 2017-03-28T18:34:09Z

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.

JustinHoldstock · 2017-03-28T19:07:18Z

src/lib/viewers/media/MediaControls.js

     * @return {void}
     */
-    initFilmstrip(url, status, aspect) {
+    initFilmstrip(url, status, aspect, interval) {


Could you give interval a default value of 1?

I don't want to default it because the default value of 1 might not be correct (see my comment above)

tonyjin · 2017-03-28T20:03:17Z

src/lib/viewers/media/Dash.js

@@ -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) {


// 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))

tonyjin reviewed Mar 28, 2017

View reviewed changes

jeremypress reviewed Mar 28, 2017

View reviewed changes

JustinHoldstock reviewed Mar 28, 2017

View reviewed changes

tonyjin reviewed Mar 28, 2017

View reviewed changes

tonyjin approved these changes Mar 28, 2017

View reviewed changes

Bryan Huh added 3 commits March 28, 2017 14:08

Filmstrip interval should be determined from rep metadata field

bf88a8f

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.

Adding comment

c4d1495

Only load filmstrip if metadata and interval exist

43c4f85

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.

bhh1988 merged commit ffc743f into box:master Mar 28, 2017

bhh1988 deleted the filmstrip_metadata branch March 28, 2017 21:16

tonyjin added the legacy-ignore label Apr 7, 2017

pramodsum mentioned this pull request Nov 14, 2017

Update: box-annotations to v0.3.0 #485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filmstrip interval should be determined from rep metadata field #23

Filmstrip interval should be determined from rep metadata field #23

bhh1988 commented Mar 28, 2017

bhh1988 commented Mar 28, 2017

tonyjin Mar 28, 2017

bhh1988 Mar 28, 2017

jeremypress Mar 28, 2017

tonyjin Mar 28, 2017

bhh1988 Mar 28, 2017

tonyjin Mar 28, 2017

bhh1988 Mar 28, 2017

bhh1988 commented Mar 28, 2017

JustinHoldstock Mar 28, 2017

bhh1988 Mar 28, 2017

tonyjin Mar 28, 2017

Filmstrip interval should be determined from rep metadata field #23

Filmstrip interval should be determined from rep metadata field #23

Conversation

bhh1988 commented Mar 28, 2017

bhh1988 commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhh1988 commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment