-
-
Notifications
You must be signed in to change notification settings - Fork 385
refactor(IVideoDevice): add Apply method #5949
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
Changes from all commits
160e5e5
d1ad1c5
4141cac
4a2927e
ef81d8f
38255ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,22 @@ | ||
| .bb-video { | ||
| .bb-actions { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: .5rem .5rem; | ||
| } | ||
|
|
||
| .bb-video { | ||
| min-height: 240px; | ||
| height: auto; | ||
| width: auto; | ||
| margin-top: 1rem; | ||
| margin: 1rem; | ||
| display: block; | ||
| } | ||
|
|
||
| .bb-image { | ||
| border: 1px solid var(--bs-border-color); | ||
| border-radius: var(--bs-border-radius); | ||
| margin-top: 1rem; | ||
| margin: 1rem 1rem 0 1rem; | ||
| display: block; | ||
| width: calc(100% - 2rem); | ||
| max-width: 640px; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -77,6 +77,41 @@ export async function close(videoSelector) { | |||||||||||||||||||||||||||||||||||||||||||||
| return ret; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export async function apply(options) { | ||||||||||||||||||||||||||||||||||||||||||||||
| let ret = false; | ||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||
| const media = registerBootstrapBlazorModule("MediaDevices"); | ||||||||||||||||||||||||||||||||||||||||||||||
| const { stream } = media; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (stream && stream.active) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const tracks = stream.getVideoTracks(); | ||||||||||||||||||||||||||||||||||||||||||||||
| if (tracks) { | ||||||||||||||||||||||||||||||||||||||||||||||
| const track = tracks[0]; | ||||||||||||||||||||||||||||||||||||||||||||||
| const settings = track.getSettings(); | ||||||||||||||||||||||||||||||||||||||||||||||
| const { aspectRatio } = settings; | ||||||||||||||||||||||||||||||||||||||||||||||
| if (options.width) { | ||||||||||||||||||||||||||||||||||||||||||||||
| settings.width = { | ||||||||||||||||||||||||||||||||||||||||||||||
| exact: options.width, | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| settings.height = { | ||||||||||||||||||||||||||||||||||||||||||||||
| exact: Math.floor(options.width / aspectRatio) | ||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+91
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Inconsistency between JS applied constraints and provided parameters. The function ignores options.height even when passed; either apply options.height to the constraints or update the interface to explicitly reflect that only width is used.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||
| if (options.facingMode) { | ||||||||||||||||||||||||||||||||||||||||||||||
| settings.facingMode = { | ||||||||||||||||||||||||||||||||||||||||||||||
| ideal: options.facingMode, | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| console.log(settings); | ||||||||||||||||||||||||||||||||||||||||||||||
| await track.applyConstraints(settings); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||
| console.error("Error apply constraints media devices.", err); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
| return ret; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| export async function getPreviewUrl() { | ||||||||||||||||||||||||||||||||||||||||||||||
| let url = null; | ||||||||||||||||||||||||||||||||||||||||||||||
| const media = registerBootstrapBlazorModule("MediaDevices"); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ public async Task Open_Ok() | |
| Context.JSInterop.Setup<string?>("getPreviewUrl").SetResult("blob:https://test-preview"); | ||
| Context.JSInterop.Setup<bool>("open", _ => true).SetResult(true); | ||
| Context.JSInterop.Setup<bool>("close", _ => true).SetResult(true); | ||
| Context.JSInterop.Setup<bool>("apply", _ => true).SetResult(true); | ||
|
|
||
| var service = Context.Services.GetRequiredService<IVideoDevice>(); | ||
| var options = new MediaTrackConstraints() | ||
|
|
@@ -44,6 +45,9 @@ public async Task Open_Ok() | |
| var close = await service.Close(".bb-video"); | ||
| Assert.True(close); | ||
|
|
||
| var apply = await service.Apply(new MediaTrackConstraints() { Width = 640, Height = 480, VideoSelector = ".bb-video" }); | ||
| Assert.True(apply); | ||
|
Comment on lines
45
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider testing failure scenarios for the Apply method. Add tests for when the JS interop call for |
||
|
|
||
| Assert.Equal("test-device-id", options.DeviceId); | ||
| Assert.Equal("user", options.FacingMode); | ||
| Assert.Equal(640, options.Height); | ||
|
|
||
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.
suggestion (bug_risk): Return value is never updated on success.
ret remains false even on success; update it (or return true) when constraints apply successfully.
Suggested implementation:
Please verify that inserting ret = true at this position fits the overall logic, meaning that the constraints have been applied by this point. You might need to adjust the placement if additional logic should occur before returning a successful value.