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

fix: OPTIC-1608: Seeking video frame by frame sometimes produces duplicated or skipped frames visually #7027

Merged
merged 13 commits into from
Feb 10, 2025

Conversation

bmartel
Copy link
Contributor

@bmartel bmartel commented Feb 5, 2025

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

There were some reports of issues using Video with certain video files, specifically with regards to skipped or duplicated frames. Looking at the commonalities of these reports and investigating further the types of files that were producing known issues, the observation was that the way video was calculating frame seeking was incorrect.

There were two problems:

  1. Visual display of frames is 1-based, while calculations of seeking frames is 0 based, because it is based on time.
  2. The step of any given frame was calculating exactly, which does not work in browsers as browser precision is at best 2ms.

Reproduction

Created a Label Studio project with the following:

Config

<View>
   <Labels name="videoLabels" toName="video" allowEmpty="true">
     <Label value="Man" background="blue"/>
     <Label value="Woman" background="red"/>
     <Label value="Other" background="green"/>
   </Labels>
   
   <!-- Please specify FPS carefully, it will be used for all project videos -->
   <Video name="video" value="$video" framerate="24"/>
   <VideoRectangle name="box" toName="video"/>
</View>

Video File

fps_24_frames_9999_video.webm

What does this fix?

  1. We properly calculate the display of frames, and the seeking of frames accordingly.
  2. We round the value of a seek step to the nearest next browser precision point.

Closes #6593

What is the new behavior?

Seeking frame by frame now correctly works for all videos.

Kapture.2025-02-05.at.17.16.54.webm

What is the current behavior?

Seeking frame by frame worked for some videos, but struggled in cases where the framerate was low or in short video clips.

Kapture.2025-02-05.at.17.11.32.webm

What feature flags were used to cover this change?

  • fflag_fix_front_optic_1608_improve_video_frame_seek_precision_short

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Video

@github-actions github-actions bot added the fix label Feb 5, 2025
Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit 471454f
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/67a6173ba3b9660008869e65

Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit 471454f
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/67a6173b0d97180008f6f78f

Copy link
Contributor

@yyassi-heartex yyassi-heartex left a comment

Choose a reason for hiding this comment

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

minor suggestion, but other wise looks good.

bmartel and others added 2 commits February 6, 2025 08:14
@bmartel
Copy link
Contributor Author

bmartel commented Feb 6, 2025

/git merge develop

Workflow run
Successfully merged: create mode 100644 web/libs/frontend-test/src/helpers/LSF/Hotkeys.ts

@bmartel bmartel merged commit 5093b7f into develop Feb 10, 2025
34 checks passed
@mandulaj
Copy link

Hey, thanks for implementing this! I was trying to test it on our videos as well as on the above 24fps video.

I build the docker image for the app with docker compose build

However, it seems that the issue is still persisting. I made sure to clear the browser cache and the version displayed is 1.17.0-dev0.

Any ideas why this might be the case?

@mandulaj
Copy link

I tried it in both Firefox and Brave browser with same behaviour

@bmartel
Copy link
Contributor Author

bmartel commented Feb 10, 2025

I tried it in both Firefox and Brave browser with same behaviour

Any ideas why this might be the case?

This is Feature Flagged to allow for rollout, so to enable the change in behaviour you will need to run your local Label Studio docker compose instance with the following environment variable set:

  • fflag_fix_front_optic_1608_improve_video_frame_seek_precision_short=true

@mandulaj
Copy link

@bmartel YES! Thats it! With the flag enabled it works flawlessly!

If I understand correctly, the frame labels are now indexed starting at 1?

@mandulaj
Copy link

Although, there is still a small bug when pausing the video during playback. It seems that temporarily, the incorrect bounding box is displayed. But I think I will open this as a separate issue. Its probably related to the index calculation, but as part of the bounding box component.

Thanks a lot!

@bmartel
Copy link
Contributor Author

bmartel commented Feb 10, 2025

@bmartel YES! Thats it! With the flag enabled it works flawlessly!

If I understand correctly, the frame labels are now indexed starting at 1?

The original intent was such that they were reflecting a 1-based index for frames, so to not disturb this it retains that behaviour. The difference now is that the seeking is done such that it takes into account the maximum precision steps that the browser inherently has with regards to the currentTime being set. This was sometimes throwing that off internally, and just not actually seeking to the time we were enforcing because it was too specific and producing "ghost" frames, where some are skipped others are duplicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frame Skipping Causes Label Misalignment in Video Annotation
5 participants