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: video crops when webcam resolution is not 480p #5

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

parth-agrawal
Copy link

@parth-agrawal parth-agrawal commented Jul 24, 2024

Description

image
Screenshot 2024-07-24 at 10 28 13 AM 1

If the demo receives webcam video of a resolution different from 480p, the demo does not scale the webcam video or canvas correctly. This appears on both the Roboflow homepage and the test page in this repository. This fix changes script.js so that canvas height is fixed to 480 pixels, but canvas width is scaled to match the aspect ratio of incoming video.

Loom Explainer

https://www.loom.com/share/53c58cccee104e2d951244d0ddda6032

Motivation

I encountered this bug because I was on a Google Meet call with a colleague while testing the demo. I determined that Google Meet was requesting 720p video from the webcam. The Roboflow demo appears to tap the same video source, so it also received 720p video. It handled this incorrectly; the video cropped to the top left corner of the video. Inference was also performed incorrectly as a result of this.

To Reproduce

  • In Chrome, open a new window. Create a Google Meet meeting. Copy the invite to this meeting.
  • Open a new window and join this meeting using a different Google account.
  • Open a third new window and navigate to the Roboflow homepage. Test the webcam demo. The video should appear cropped to the top left corner.
  • NOTE: this bug may not reproduce if you do not open each of these in a new window.

Fix

Screenshot 2024-07-24 at 10 29 04 AM

Summary of changes

webcamInference()

  • receive actual video dimensions from the video stream instead of assuming 480p (640x480) video
  • calculate videoAspectRatio as width/height
  • move getCoordinates() calculation outside of video render loop
  • resize canvas based on getCoordinates() calculation of canvas width and height

getCoordinates()

  • accept a new parameter, videoAspectRatio
  • keep canvas height to be a fixed 480 pixels, but dynamically set canvas width based on received videoAspectRatio
  • set sWidth to the incoming true webcam video width, which fixes the scalingRatio calculation
  • prior calculations using canvasRatio and imageRatio are not necessary given that we know the true webcam video dimensions

getBase64Image()

  • delete from file, unused

imageInference()

  • delete from file, unused

To Test the Fix

  • Follow the bug reproduction steps above. The image should scale correctly based on the incoming video resolution (720p). The incoming video dimensions can be logged to observe this more closely.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

script.js Outdated Show resolved Hide resolved
@parth-agrawal parth-agrawal marked this pull request as ready for review July 24, 2024 15:46
@MadeWithStone
Copy link
Collaborator

@parth-agrawal Thank you for bringing the issue to our attention and for the contribution. At the moment we'd rather the demo keep a constant pixel resolution so that it works well with our content management system and we can ensure layouts on a variety of device sizes. I pushed a separate fix to the repo which handles differing webcam sizes while maintaining container sizes for layout (would be great if you could test that :)).

However, I'm sure other users will want the scaling containers you show in your PR so it would be great if you instead moved those to separate html and js files for a separate "scaling" demo (maybe update the README while your at it). At which point you can request a review from me and I'd be happy to merge that demo into the main repo.

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.

3 participants