840244 - Android getUserMedia captured video sideways on some phones

Status: RESOLVED FIXED

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

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

This is turning out to be a lot more exciting than I had hoped.

First off, it turns out that the capture code doesn't use the usual Android MediaRecorder classes directly for recording.  Instead, it gets preview callbacks in onPreviewFrame, and passes those preview frames back to native C++ code in the video engine.

Variants of this rotation problem on Samsung Galaxy S devices are common knowledge around the net, as evidenced by a Google search.

Things that one might hope would fix it, but don't:

camera.setDisplayOrientation():  As the API docs point out:

"This does not affect the order of byte array passed in onPreviewFrame(byte[], Camera), JPEG pictures, or recorded videos."

camera.parameters.setRotation(): turns out that as implemented by Samsung, this really just sets a rotation constant in any associated EXIF data, it doesn't change the format.

Things to try next:

* see if tweaking setting something like android:orientation to "vertical" in the manifest makes a difference (would surprise me if this works, but it's easy to test :-)

* bump our minimum API level to 11 and switch from using a SurfaceHolder to a TextureView as suggested by snorp elsewhere, I think.  Not only does this look like it might fix the problem, I'd also expect it to be faster.  Not sure how much work this would be.

* see if switching to MediaRecorder and using setOrientationHint() would do the trick.  Again, unclear how much work this would be.  Seems like it also has the potential to cause other issues.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

It would be particularly interesting to know if the LG Nexus 4 has this problem.  If not, we could consider punting on the S3 for wow.  I hope to be getting one soon, but if someone who already has one can test it before I do, that would rock.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

And, more generally, how well video works on that phone.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Turns out the Nexus 4 also has this issue.  

However, Florian made the excellent observation that if one holds the phone in landscape mode, it's not an issue.  So he and I chatted a bit, and he's going to work on getting the social API demo to look nicer in landscape mode.  Thanks, Florian!

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

ekr also suggested that another possible workaround would be to use CSS to rotate the image on both sides.  jesup mentioned that adding "a=orientation" to the SDP signaling has been used as a way to communicate that a rotated image is being transmitted.  derf pointed out, however, that that'd be slow since the CSS transform isn't currently GPU-accelerated.

My current take is that just setting up the demo to look best sideways (and maybe not auto-rotate, if possible) is likely to be enough.

That said, probably the best next step will be to bump the API-level and try the TextureView.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Dan Mosedale (:dmose) from comment #5)
> ekr also suggested that another possible workaround would be to use CSS to
> rotate the image on both sides.  jesup mentioned that adding "a=orientation"
> to the SDP signaling has been used as a way to communicate that a rotated
> image is being transmitted.  derf pointed out, however, that that'd be slow
> since the CSS transform isn't currently GPU-accelerated.
> 
> My current take is that just setting up the demo to look best sideways (and
> maybe not auto-rotate, if possible) is likely to be enough.
> 
> That said, probably the best next step will be to bump the API-level and try
> the TextureView.

It's SurfaceTexture that you would want to use. We can render that in OpenGL through the layer system and performance should be good. You don't need to bump the API level, you can use it conditionally.

We can render that in OpenGL, and performance should be pretty good

Comment 7

[Gian-Carlo Pascutto [:gcp]]

The problem is this: setDisplayOrientation does not affect the data in onPreviewFrame. Neither does setRotate to anything useful. Changing the orientation of our entire app isn't possible. So 99% of the advice about this out there is useless for us.

Similarly, using a SurfaceTexture doesn't change anything, because it's the data that we comes in that we care about, not what's on the screen. We can use a SurfaceTexture to do the rotation, but then we must read it in again before passing it down. This means using getBitmap and doing a dance to get data[] again (and probably more format conversions when we have to encode the video).

Alternatively, we can pass the raw, unrotated NV21 data down to WebRTC C++ code together with a constant identifying by how much it needs to be rotated. That code can use libyuv to do this rotation. It's already in the tree and there's even NEON optimized versions available. (Compare to rotating the NV21 data in Java: needs quite some extra coding because there is no support available)

The latter, i.e. using libyuv to rotate at the C++/WebRTC level looks strongly preferable to me at this point.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

It seems we should just call SetCaptureRotation in video_capture_android.cc, perhaps just after we probe the cameras. This passes it to libyuv already, so the rotation is free.

Only need some code to fetch the camera orientation and do the math at init time.

However when I try this we segfault. Will need to look with a debugger what's up.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Vital information:
http://code.google.com/p/webrtc/issues/detail?id=424

Comment 10

[Gian-Carlo Pascutto [:gcp]]

We'll need to take a patch from upstream, the code was significantly fixed to deal with this. The code we have *can not* rotate 90/270 degrees correctly.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix the handling of rotation in video_capture_impl

This should make the rotation work correctly. Inferred from reading the above-mentioned Android issue 424.

We need some code to determine the rotate amount, but that should be easy now.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

I haven't had a chance to look at this in detail, however, I did uncomment the SetRotation line and tried it, and while it works on the video-only gUM test, it fails on the video/audio gUM test, as well as on both calls.  So I'm suspecting this is going to need some tweaking before it gets an r+.  :-)

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

And by both calls, I mean both the backup and production video call demos.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Calculate the rotation amount and apply

This also fixes a bug in stopping and restarting the camera when rotation is active.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 716035 [details] [diff] [review]
Patch 1. Fix the handling of rotation in video_capture_impl

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

r=dmose for landing on alder

::: media/webrtc/trunk/src/modules/video_capture/main/source/android/video_capture_android.cc
@@ +492,5 @@
>      }
>    }
> +
> +  //SetCaptureRotation(kCameraRotate270);
> +

Since this gets overwritten by the next patch, leaving it here in this one seems ok.

::: media/webrtc/trunk/src/modules/video_capture/main/source/video_capture_impl.cc
@@ +281,5 @@
>          }
>  
> +        // Rotating resolution when for 90/270 degree rotations.
> +        if (_rotateFrame == kRotate90 || _rotateFrame == kRotate270)  {
> +            target_width = abs(height);

I'm not sure why abs() is necessary, in the sense that if this code is passing around negative heights, that sounds pretty strange.  But it seems implausible that there's much of a perf win to be had here, and it's not going to hurt semantically, so it works for alder.  If this patch gets uplifted, I'd propose that we assert if height is negative, unless that's just a "feature" of the way this code module works.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 716644 [details] [diff] [review]
Patch 2. Calculate the rotation amount and apply

In general, this patch looks great. If you wanted to get someone else to review the rotation math bits as well, that wouldn't hurt.  But having tried the patch, it clearly to work on the phone that we care about for alder, so that's not a requirement for it land.  r=dmose

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

So despite having given both of those patches an r+, I'm still on the fence about the best way to land it.  To wit:

Because it makes the aspect ratio problem worse, it feels like it makes the user experience of portrait mode just differently weird: notably better in one way and notably worse in another.

My current suspicion after playing around with gUM and both calling demos is that fixing the aspect ratio thing is likely to require at least the platform fix gcp and I discussed in IRC and quite possibly one or more content-level fixes to the production demo.

Separately from all this, the landscape fixes which we need anyway for the production demo either are incomplete or haven't yet been deployed. 

From a holistic perspective, I think our immediate choice here is between:

* get the content folks to prioritize landscape mode fixes and live with landscape-mode only

* finish the aspect-ratio bug, and once we have a patch for that in hand, ask the content folks to prioritize any necessary fixes to portrait-mode that the rotation and aspect changes require and live with portrait-mode only, then land the three platform patches and any necessary content fixes

The second choice here seems to notably more moving parts, and also seems likely to require more bandwidth from the demo content folks, and therefore more risk.  So my take at his point is that living with landscape mode is the way to go, we should wait to land this patch.

One of the things effecting my decision here is that I'm surprised by how many call-reliability issues we're still seeing, and that they appear to have parts in both the content and platform code.  My hope was that uplifting jesup's patch would have fixed most of them, but I'm still seeing a frustratingly high number of failed calls, and I get the sense from IRC that other people are too.  So fixing those seems like it's worth prioritizing above this on both the content and platform sides.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Note also that even if we were to take all of the portrait-mode fixes described above, and the content team made landscape mode look nice too, switching from portrait mode to landscape mode during a call would require further changes to the platform code.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

My bugmail access is likely to be spotty tomorrow and over the weekend.  However, people should feel free to call if they want to discuss, or if there's code I can productively write to change this equation.

Ultimately, I think Standard8 is in the best position to make a final decision about how to move forward here.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

>in the sense that if this code is passing around negative heights, that sounds 
>pretty strange

The webrtc.org code seems to use negative heights to signal "vertically flipped". Why it needs to do that I don't know, but the patch is intended to not break when that happens.

>One of the things effecting my decision here is that I'm surprised by how many 
>call-reliability issues we're still seeing, and that they appear to have parts in 
>both the content and platform code...
>or if there's code I can productively write to change this equation.

Are there indications that these issues are Android specific, or it it just general WebRTC issues?

I can perhaps improve the aspect ratio problems by the patch we discussed, but I think we also both understand that any work on alder is reasonably likely to be throw-away at this point (especially as we're fixing things that have upstream fixes/changes). So if it's not going to be used that's even a greater waste of time.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 3. Report true resolutions, and convert-back on startcapture

This changes the device capability detection to know about the rotated camera, and makes StartCapture smart about associating the rotated ones back to the actual camera res.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 717189 [details] [diff] [review]
Patch 3. Report true resolutions, and convert-back on startcapture

I'm not totally convinced I'm understanding the flow correctly here, in part because it looks like this patch hasn't been rebased against what's currently in mozilla-central, so it's a little hard to figure out what's going on in Splinter.

However, if I'm understanding it correctly, this patch assumes that the camera is mounted sideways to the phone like Nexus 4 or Samsung, which presumably won't be true in the general case.  If that's incorrect, what am I missing?

Comment 23

[Gian-Carlo Pascutto [:gcp]]

>However, if I'm understanding it correctly, this patch assumes that the camera is 
>mounted sideways to the phone like Nexus 4 or Samsung, which presumably won't be 
>true in the general case.

No, not at all. This code probes the rotation of the camera, of the device, and infers the rotation from that. That's what GetRotateAmount(android.hardware.Camera.CameraInfo info) does.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 717189 [details] [diff] [review]
Patch 3. Report true resolutions, and convert-back on startcapture

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

r=dmose with that comment tweaked.  Note that I haven't actually applied this patch and tested it myself; I'm assuming that you've done that on trunk and everything is working. Presumably you'll want to convince a module peer to either delegate their review to me, or for them to have a quick look at it as well.

::: media/webrtc/trunk/src/modules/video_capture/main/source/video_capture_impl.cc
@@ +263,5 @@
> +    WebRtc_Word32 width = frameInfo.width;
> +    WebRtc_Word32 height = frameInfo.height;
> +    // These need to be inverted, we rotated the reported camera
> +    // sizes to what the app will get, but the source for our
> +    // rotation doesn't have it applied yet, so undo that here.

This comment doesn't seem very clear.  I think the use of more explicit nouns will likely help (eg "we rotated" could state what code did the rotation, and "the source for our rotation" is vague as well.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Retrive and apply the needed rotation amount for the camera.

Simplified patch, making most review comments outdated; the underlying problem was mostly fixed in the webrtc.org v3.20 update, which has landed on m-c but was not on alder. These are the remaining bits.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 731901 [details] [diff] [review]
Patch 1. Retrive and apply the needed rotation amount for the camera.

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

::: media/webrtc/trunk/webrtc/modules/video_capture/android/java/org/webrtc/videoengine/VideoCaptureDeviceInfoAndroid.java
@@ +274,5 @@
>                          default:
>                              // From Android 2.3 and onwards)
> +                            if(android.os.Build.VERSION.SDK_INT>8) {
> +                                cameraId = device.index;
> +                                camera = Camera.open(device.index);

you could do:
  camera = Camera.open(cameraId = device.index);

but this is probably more readable

Comment 27

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a53e33b99e42

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a53e33b99e42

Comment 29

[Jason Smith [:jsmith]]

Not building WebRTC in Android by default yet, so can't verify.