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.

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

Implements 'Jump frame by frame' in video #823

Merged
merged 24 commits into from
Jun 18, 2024

Conversation

ToukL
Copy link
Contributor

@ToukL ToukL commented May 2, 2024

Splitted from here #821

Workaround to navigate frame by frame (forward and backward). Only works in Chrome for now, needs more tests.

@mzur
Copy link
Member

mzur commented May 3, 2024

Reference to my earlier response to this: #821 (comment)

Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

I think this still includes the "jump by seconds" code.

@ToukL
Copy link
Contributor Author

ToukL commented May 3, 2024

I think this still includes the "jump by seconds" code.

Ah yes I based this branch on "jump-by-seconds" but I can rebase on master, it would be cleaner indeed

…gation

Add "show previous frame" and "show next frame" buttons in video navigation

Add "show previous frame" and "show next frame" buttons in video navigation
@mzur mzur self-requested a review May 3, 2024 11:39
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

I see that this could be useful, although it may not be correct all the time. Since this is an "experimental" feature then, I'd rather make it opt-in in the annotation tool settings. This way, we could solve four issues at once:

  1. Explain to the users that the method may not always be correct (in the manual).
  2. Show non-Chrome users that the feature exists but is not available for them (by being disabled).
  3. Allow users who don't need the feature to hide some of the new arrow buttons that were now introduced to clean up their user interface.
  4. Set left/right arrow as keyboard shortcut to jump frames only if the feature is disabled. If the feature is disabled, the old behavior is still active (arrows switch videos). If the feature is enabled Shift+arrows could be used to switch videos.

So if you agree with this, here are my comments:

  • Please check for the availability of requestVideoFrameCallback when the video annotation tool loads. If it is not available, the feature is disabled and the settings button (see below) is disabled, too, so it cannot be clicked. This way you don't have to check for the availability of the method on each frame jump any more.
  • Add a settings button that enables the feature. It should be disabled by default and labeled as "experimental" with a link to the manual. If the feature is disabled, the next/previous buttons to jump frames should be hidden.
  • Similar to the other PR, all next/previous buttons should belong to the same btn-group.
  • As discussed, the jump by frame buttons should use a different icon.
  • Similar to the other PR, add a section to the manual article that explains the feature, why it is "experimental" (because it may not always be correct) and that it is only available in Chrome.
  • You should prevent "double clicks" on the jump by frame buttons, i.e. while the next frame is seeked, the buttons should be disabled. Ideally, you should emit an event from the videoScreen component that calls the seek() method of the videoContainer. This will set this.seeking=true which shows a little loading spinner. While this.seeking=true, the buttons are disabled. The this.seeking state is also available in the videoScreen component and in videoPlayback. However, this is not so easy here, as the video is "secretly" seeked to find the next frame... Maybe you have to add an extra method to the videoContainer to control this.seeking with events from the videoScreen component.

@ToukL
Copy link
Contributor Author

ToukL commented May 7, 2024

  • Please check for the availability of requestVideoFrameCallback when the video annotation tool loads. If it is not available, the feature is disabled and the settings button (see below) is disabled, too, so it cannot be clicked.

When you say "when the video annotation tool loads" is it in the loadVideo() method of the videoContainer or elsewhere (in the views part maybe) ?

@mzur
Copy link
Member

mzur commented May 7, 2024

The appropriate location would be in the created() method of the videoContainer. You can set a new attribute (e.g. this.supportsJumpByFrame) there and pass this on to the videoScreen and settings tab as props.

@mzur
Copy link
Member

mzur commented May 17, 2024

Please re-request my review once you are finished with all your changes so I know that I should have a look 😉

@ToukL ToukL requested a review from mzur May 22, 2024 09:09
@ToukL
Copy link
Contributor Author

ToukL commented May 22, 2024

Please re-request my review once you are finished with all your changes so I know that I should have a look 😉

I just did (sorry I was a bit busy these days so it took me a while...)

I am not absolutely sure for the last point (the call to seek method to prevent double clicks). I only added events in VideoScreen that call the seek() method if this.seeking=false, according to my tests it seems to catch what we want without having to add an extra method in VideoContainer but maybe I'm missing something, tell me what you think :)

Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

It does what I wanted although I don't understand how 😄

Can you please merge the new master and resolve the conflicts? I also decided that the "previous/next video" buttons should now be moved to a separate btn-group at the left like this:

<div v-if="showPrevNext" class="btn-group">
    <control-button
        icon="fa-step-backward"
        :title="jumpByFrameEnabled ? 'Previous video 𝗦𝗵𝗶𝗳𝘁+𝗟𝗲𝗳𝘁 𝗮𝗿𝗿𝗼𝘄' : 'Previous video 𝗟𝗲𝗳𝘁 𝗮𝗿𝗿𝗼𝘄'"
        @click="emitPrevious"
        ></control-button>
    <control-button
        icon="fa-step-forward"
        :title="jumpByFrameEnabled ? 'Next video 𝗦𝗵𝗶𝗳𝘁+𝗥𝗶𝗴𝗵𝘁 𝗮𝗿𝗿𝗼𝘄' : 'Next video 𝗥𝗶𝗴𝗵𝘁 𝗮𝗿𝗿𝗼𝘄'"
        @click="emitNext"
        ></control-button>
</div>
<div class="btn-group">
    <control-button
        v-if="enableJumpByFrame"
        :disabled="seeking"
        icon="fa-caret-square-left"
        title="Previous frame 𝗟𝗲𝗳𝘁 𝗮𝗿𝗿𝗼𝘄"
        v-on:click="emitPreviousFrame"
        ></control-button>
...

Then there is a separate group to switch videos and a group to control the video playback/jumping.

resources/assets/js/videos/videoContainer.vue Outdated Show resolved Hide resolved
resources/assets/js/videos/components/videoScreen.vue Outdated Show resolved Hide resolved
resources/assets/js/videos/components/videoScreen.vue Outdated Show resolved Hide resolved
resources/assets/js/videos/components/videoScreen.vue Outdated Show resolved Hide resolved
resources/assets/js/videos/components/videoScreen.vue Outdated Show resolved Hide resolved
@ToukL ToukL requested a review from mzur June 4, 2024 13:32
@ToukL
Copy link
Contributor Author

ToukL commented Jun 4, 2024

Hello Martin,
I re-requested the review but there is still an issue with the seek event that may freeze if the user click too many times (I don't know if you saw my comment above)

@ToukL ToukL requested a review from mzur June 13, 2024 15:06
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

We are getting there, thanks! I've tweaked the manual articles a little bit. Also, now I observe the following behavior: When I keep the left/right arrow key pressed, the seeking starts and stops continuously but the video display is only updated once I release the key. It seems like it actually seeks multiple frames and then renders the final result. Before, I could see the rendered frame updating on each step while keeping the key pressed.

@ToukL ToukL requested a review from mzur June 18, 2024 12:27
Copy link
Member

@mzur mzur left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks! May I mention you and your project (called FUTURE-OBS, right?) in the BIIGLE newsletter when I announce the new features? Is there a project website that I can link?

@mzur mzur merged commit 1ed7ba2 into biigle:master Jun 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants