Closed Bug 1003274 Opened 6 years ago Closed 5 years ago

Can't require/select camera using width/height/frameRate

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jib, Assigned: jib)

References

(Blocks 1 open bug)

Details

(Whiteboard: [p=1])

Attachments

(2 files, 7 obsolete files)

15.85 KB, patch
jib
: review+
Details | Diff | Splinter Review
16.26 KB, patch
jib
: review+
Details | Diff | Splinter Review
Left out one patch in Bug 907352 in the interest of landing. Will add it here.

It ties in camera selection to use the new width/height/framerate code.

To clarify:
- Bug 907352 lets you control your camera, set a certain width/height/frameRate.
- This issue lets you also select between multiple cameras (if you have them)
  using these parameters, and also fail using the "require" key.
Whiteboard: [p=1]
Target Milestone: --- → mozilla33
Priority: -- → P2
Target Milestone: mozilla33 → mozilla35
Attached patch refactor template types (obsolete) — Splinter Review
Cleaning up the template types helps a bit in preparation for next patch.
Attachment #8494860 - Flags: review?(rjesup)
This is an updated version of the dusted-off patch "missing" from Bug 907352.

While we already support picking the right capability-set in a selected camera so that it adheres to constraints, actual camera *selection* was missing. 

With this patch, the constraints algorithm for camera selection is finally wired into the camera capabilities code, so that a camera's potentially numerous sets of width/height/frameRate capabilities are taken into account when choosing the or rejecting the camera in the first place

This does bupkis on a mac, so I'll need to do testing on Windows next, and write some mochitests for this, although I'll probably update the constraint syntax first (Bug 1033885) so I'll only have to write them once.
Attachment #8494866 - Flags: review?(rjesup)
Attachment #8494860 - Flags: review?(rjesup) → review+
Comment on attachment 8494866 [details] [diff] [review]
clue camera selection in to width/height/frameRate code

Review of attachment 8494866 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +228,5 @@
> +  for (int i = 0; i < num; i++) {
> +    candidateSet.AppendElement(i);
> +  }
> +
> +  for (uint32_t j = 0; j < aConstraintSets.Length(); j++) {

size_t

@@ +229,5 @@
> +    candidateSet.AppendElement(i);
> +  }
> +
> +  for (uint32_t j = 0; j < aConstraintSets.Length(); j++) {
> +    for (uint32_t i = 0; i < candidateSet.Length();) {

size_t
Also, spaces or a /* comment */ for the loop iterator that's missing and handled in the loop

::: dom/media/MediaManager.cpp
@@ +427,3 @@
>  {
> +  // Interrogate device-inherent properties first.
> +  for (uint32_t i = 0; i < aConstraintSets.Length(); i++) {

I think size_t is more correct for .Length(); from nsTArray_base:
  typedef size_t size_type;
  typedef size_t index_type;

@@ +431,5 @@
> +    if (c.mFacingMode.WasPassed()) {
> +      nsString s;
> +      GetFacingMode(s);
> +      if (!s.EqualsASCII(dom::VideoFacingModeEnumValues::strings[
> +          uint32_t(c.mFacingMode.Value())].value)) {

static_cast<>?  (syntactic sugar, of course)

@@ +439,4 @@
>      nsString s;
> +    GetMediaSource(s);
> +    if (!s.EqualsASCII(dom::MediaSourceEnumValues::strings[
> +        uint32_t(c.mMediaSource)].value)) {

static_cast<>?
Attachment #8494866 - Flags: review?(rjesup) → review+
Updated commit msg.
Attachment #8494860 - Attachment is obsolete: true
Attachment #8495611 - Flags: review+
Updated nits. Carrying forward r=jesup.
Attachment #8495614 - Flags: review+
Up for storage (my Mac is going to service tomorrow)
Attachment #8509265 - Attachment is obsolete: true
Attachment #8509267 - Attachment is obsolete: true
Unbitrot. Carrying forward r=jesup.
Attachment #8495611 - Attachment is obsolete: true
Attachment #8510784 - Flags: review+
Unbitrot. Carrying forward r=jesup.

These were getting really stale so I'm getting ready to land them before Bug 1033885.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d8cb7bdf359d
Attachment #8494866 - Attachment is obsolete: true
Attachment #8495614 - Attachment is obsolete: true
Flags: needinfo?(jib)
Attachment #8510788 - Flags: review+
Attachment #8510788 - Attachment description: Part 1: refactor template types (3) r=jesup → Part 2: clue camera selection in to width/height/frameRate code (3) r=jesup
Flags: needinfo?(jib)
Flags: needinfo?(jib)
Added virtual stub to compile on B2G. Carrying forward r=jesup.

Try - https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1148e4f741a
Attachment #8510788 - Attachment is obsolete: true
Attachment #8510822 - Flags: review+
Green try. Filed Bug 1088621 on problem about how to test this stuff.
Flags: needinfo?(jib)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d48c270e62c9
https://hg.mozilla.org/mozilla-central/rev/c2233448b59c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
You need to log in before you can comment on or make changes to this bug.