Open Bug 1061704 Opened 10 years ago Updated 2 years ago

Cleanup facingMode implementation to work on internal camera stack properties instead of through device name snooping

Categories

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

defect

Tracking

()

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(1 file)

Proper fix for Bug 1060708. Possibly entangled with Bug 1003274.
WIP moved here from Bug 1060708.

I already know that exposing a boolean is insufficient, since I wont be able to tell environment-facing cameras from unknown-facing cameras, so I'll need to make a new enum for all the standard facingMode types and return that at least.
Attachment #8482753 - Flags: feedback?(gpascutto)
"Basically, there are two types of properties, global inherent ones for the physical camera (facingMode, mediaSource), and ones that change for each virtual "capability" (width/height)."
(From the original bug)

What does making this distinction get you?

I'd treat each physical attribute + virtual capability as a "capability" tuple, and filter on that. This seems like it would work well with constraints?
Flags: needinfo?(jib)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> What does making this distinction get you?

It's not my distinction. I'm trying to stay faithful to what's in the source already:

- Orientation and facingMode seem to be considered inherent properties of one "capture device",
  i.e. under VideoCaptureModule::DeviceInfo [1]

- while each such capture device appears to inhabit many "capability sets" consisting of
  width, height and frameRate, etc. under VideoCaptureCapability [2]

If that's the organization - and it's just buried under layers of code - then surfacing things so people can query a MediaDevice directly for its direction and orientation without enumerating its set of logical "capability-sets" seems structurally coherent at least.

> I'd treat each physical attribute + virtual capability as a "capability"
> tuple, and filter on that. This seems like it would work well with
> constraints?

Yes by fixing Bug 1003274 first - plumbing capabilities into the camera-selection process - we may not need to surface facingMode all the way up, if constraints is the sole beneficiary of this.
Flags: needinfo?(jib)
Thanks. I see now that the issue is that for example MediaEngineWebRTCVideoSource::ChooseCapability comes too late in the process, i.e. it only does its filtering after a camera has already been selected.

As for orientation being a property of the capture device, I was wondering how exposed this needs to be exactly. From the point of view of the current Android code the rest of the program has no business with this, as the Camera driver itself will rotate the picture according to the orientation and the current device rotation. 

But combined with constraints, this brings me to an interesting problem:
A webpage makes a required constraint of a 640x480 image. We find one such camera and start the stream. Now the phone rotates and the image turns into an 480x640 one, thereby no longer fitting the constraint. What do we do?
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> Thanks. I see now that the issue is that for example
> MediaEngineWebRTCVideoSource::ChooseCapability comes too late in the
> process, i.e. it only does its filtering after a camera has already been
> selected.

Yes until Bug 1003274 is fixed.

> As for orientation being a property of the capture device, I was wondering
> how exposed this needs to be exactly. From the point of view of the current
> Android code the rest of the program has no business with this, as the
> Camera driver itself will rotate the picture according to the orientation
> and the current device rotation. 

My head is spinning - no pun intended - when it comes to how orientation may or may not work. Oddly, the two capture devices that gUM sees on my S4:

 Camera 0, Facing back, Orientation 90,
 Camera 1, Facing front, Orientation 270,

I have no idea what code ultimately pairs this down to two choices, or which one is chosen or if it matters.

> But combined with constraints, this brings me to an interesting problem:
> A webpage makes a required constraint of a 640x480 image. We find one such
> camera and start the stream. Now the phone rotates and the image turns into
> an 480x640 one, thereby no longer fitting the constraint. What do we do?

Good question. Reading the spec [1] it seems to mandate that we fire an overconstrained event in this case. Does that make sense? If not, then we should probably raise that question on the list, to see if this is expected.

[1] http://dev.w3.org/2011/webrtc/editor/getusermedia.html#attributes-8
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> Now the phone rotates and the image turns into an 480x640 one,
> thereby no longer fitting the constraint. What do we do?

Are you sure this can happen? Using the native camera app on my S4, I observe that any video I record takes its dimensions from the position I start out recording in. So if I start a recording in portrait mode and immediately flip it to landscape to record everything, then everything appears rotated in the final video, i.e. the recording source does not appear to adapt during a recording.
>I have no idea what code ultimately pairs this down to two choices, or which one is chosen or if it matters.

I'm not sure why you say "pair down to two choices". There are two cameras, so you get 2 choices. As you already said yourself, the facing and orientation are physical camera properties.

>Does that make sense? If not, then we should probably raise that question on the list, to see if this is expected.

I think that makes sense, yes. Webpages shouldn't care unless they really can't handle the new format.

>Are you sure this can happen?

Why don't you take Firefox and try it out? :-)

https://bugzilla.mozilla.org/show_bug.cgi?id=877244
(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> I'm not sure why you say "pair down to two choices".

Oh, pilot error. I meant to take out that sentence. I saw double the amount of cameras for a sec from a faulty experiment I made.

What I meant to ask was: how can the front camera be at a 90 degree angle and the back cameras be at 270 at the same time? Some odd way of counting? When I flip between them their orientation sure seem the same.

> >Are you sure this can happen?
> 
> Why don't you take Firefox and try it out? :-)
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=877244

Duh, yes it does rotate. :-) I've made a note in Bug 910249 to fire overconstrained on this.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)

> What I meant to ask was: how can the front camera be at a 90 degree angle
> and the back cameras be at 270 at the same time? Some odd way of counting?
> When I flip between them their orientation sure seem the same.

No, they're really mounted sideways in the phone. For some background, see: http://www.morbo.org/2013/02/why-twitter-pictures-are-sideways.html

The software has to compensate for it.
Depends on: 1003274
Whiteboard: p=1
Comment on attachment 8482753 [details] [diff] [review]
WIP on exposing facingMode from cameras

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

::: media/webrtc/trunk/webrtc/modules/video_capture/device_info_impl.h
@@ +40,5 @@
>          const char* deviceUniqueIdUTF8,
>          VideoCaptureRotation& orientation);
> +    virtual int32_t GetFrontFacing(
> +        const char* deviceUniqueIdUTF8,
> +        bool& frontFacing);

I think it makes sense to combine this with the orientation, i.e. extend GetOrientation to also return that bool, instead of making separate calls. They're a fairly logical unit.
Attachment #8482753 - Flags: feedback?(gpascutto) → feedback+
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Note that his is a technical dept / internal cleanup bug. facingMode was added in Bug 882145.
Summary: Expose facingMode properly in camera stack instead of doing camera device name string compare → Internal cleanup: get facingMode up from camera stack instead of camera device name
Summary: Internal cleanup: get facingMode up from camera stack instead of camera device name → Internal cleanup: get facingMode up from camera stack instead of through camera device name logic
Summary: Internal cleanup: get facingMode up from camera stack instead of through camera device name logic → Cleanup facingMode implementation to work on proper camera stack properties instead of through device name snooping
Summary: Cleanup facingMode implementation to work on proper camera stack properties instead of through device name snooping → Cleanup facingMode implementation to work on internal camera stack properties instead of through device name snooping
Not sure this needs to be done.
Rank: 35 → 50
Priority: P3 → P5
Whiteboard: p=1
See Also: → 1451798
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: