970183 - [B2G getUserMedia] Camera always callback landscape frames

Status: RESOLVED FIXED

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

Description

[StevenLee[:slee]]

gUM should listen to orientation sensor and tell camera to callback correct video frames.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

FYI corresponding bugs for Firefox for Android implementation:

https://bugzilla.mozilla.org/show_bug.cgi?id=840244
https://bugzilla.mozilla.org/show_bug.cgi?id=885031
https://bugzilla.mozilla.org/show_bug.cgi?id=891158

Comment 2

[Chiajung Hung [:chiajung]]

(In reply to StevenLee[:slee] from comment #0)
> gUM should listen to orientation sensor and tell camera to callback correct
> video frames.
How to define correct video orientation?
On B2G/Android devices the sensor output frame in specific orientation base on how the OEM assemble the camera, and the only way to get the knowledge is to get camera parameter about the orientation.
If we want to post-processing it to make it always "correct", we will have to rotate it frame-by-frame which is not realistic. 

If we want to show it "correctly" in preview, the lightest way is to change the video tag's CSS rule, to make layout engine rotate the buffer on composition. If we want to encode it "correctly", we may request encoder rotate it when encoding. I am not sure whether android's Camera.setDisplayOrientation(Java call) helps, if it helps, we can implement a function like it.

Another way is to rotate the frame on the way copy frame to GIPS. The problem is that MediaPipeline do not have the orientation info and we will have to find a way to propagate the info and current IncomingFrame does not include orientation.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

I would recommend you read the Firefox for Android bug I linked (and maybe this story: http://www.morbo.org/2013/02/why-twitter-pictures-are-sideways.html).

It answers all your questions: yes, you need to probe the camera to know how it's mounted in the phone/tablet, and also probe how the phone/tablet is *currently* oriented. Yes, you need to rotate it frame by frame. No, it's not unrealistic, the webrtc code and libyuv have *very* fast code to do this already, you just need to plug in the rotation you calculated in SetCaptureRotation. No, rotating it on display does not work, and setDisplayOrientation is entirely useless.

Comment 4

[Chiajung Hung [:chiajung]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #3)
> I would recommend you read the Firefox for Android bug I linked (and maybe
> this story:
> http://www.morbo.org/2013/02/why-twitter-pictures-are-sideways.html).
> 
> It answers all your questions: yes, you need to probe the camera to know how
> it's mounted in the phone/tablet, and also probe how the phone/tablet is
> *currently* oriented. Yes, you need to rotate it frame by frame. No, it's
> not unrealistic, the webrtc code and libyuv have *very* fast code to do this
> already, you just need to plug in the rotation you calculated in
> SetCaptureRotation. No, rotating it on display does not work, and
> setDisplayOrientation is entirely useless.
Yes, it seems it only setTransform in SurfaceTexture, which only affect display not the captured frame.

Comment 5

[StevenLee[:slee]]

I tried gcp's suggestion and do the rotation in GIPS by libyuv. It works and here is the testing results by using the benchmark from bug 890419. I ran the test on ungai. When doing extra-rotation, the percentage of fps decreasing is about 5~7%.

without rotate
Resolution@Rate FPS
176x144@60      57.317100
320x240@60      21.695000
640x480@60      6.489200
1280x720@60     2.712370

with rotate
Resolution@Rate FPS
176x144@60      53.342400
320x240@60      21.318100
640x480@60      5.984290
1280x720@60     2.573230

Comment 6

[Gian-Carlo Pascutto [:gcp]]

That's more than I would have expected but it may be due to a worse cache order. Did you add an extra call or did you modify the existing format-conversion/scaling call in the WebRTC code to also do the rotation in one go?

It sits here:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc#291

Comment 7

[StevenLee[:slee]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> That's more than I would have expected but it may be due to a worse cache
> order. Did you add an extra call or did you modify the existing
> format-conversion/scaling call in the WebRTC code to also do the rotation in
> one go?
No, I just add the following code after [1].
mPtrViECapture->SetRotateCapturedFrames(mCapId, RotateCapturedFrame_90);

[1] http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#367

Comment 8

[StevenLee[:slee]]

Attachment: rotate_video - WIP

This is a WIP patch. The concept is when sipcc gets connected, it creates MediaConduit and adds into PeerConnectionMedia. On B2G, it gets the camera mount angle from MediaManager by using window id. Then it calls SetRotateCapturedFrames through WebrtcVideoConduit.

Comment 9

[Eric Rescorla (:ekr)]

Stephen,

The frames are also rotated in the preview window.

It seems like we want to do the derotation on the gUM side.

Comment 10

[StevenLee[:slee]]

(In reply to Eric Rescorla (:ekr) from comment #9)
> Stephen,
> 
> The frames are also rotated in the preview window.
> 
> It seems like we want to do the derotation on the gUM side.
The original frame comes from the camera should be landscape mode(at least on unagi and peak). My patch only adjusts the frames that will be sent to the remote peer. So that the preview window(local stream) is still in landscape mode. I tried some ways to make camera output portrait mode frames(by setting parameters of "rotation", "orientation", ...) but failed. For preview window, camera app seems rotate the canvas by using css.

Comment 11

[Eric Rescorla (:ekr)]

Maybe we have to do this as a hack, but it's not what we want, because
apps need to be able to render gUM streams directly.

Comment 12

[Chiajung Hung [:chiajung]]

If we want to make camera output always shows correctly, there are several choice for GUM:
(1) rotate the GraphicBuffer inside GonkNativeWindow by libyuv
(2) make layer system knows the orientation to show a specific layer
(3) let app change the layout by CSS

The problem of (1) is GraphicBuffer from camera do not have cache, the performance may not acceptable, though we need not to rotate the buffer again for encoder. (2) maybe the best way, but the frame to be encode will still need rotation. (3) is only possible for local Camera app.

To do (2), we have to make GrallocImage carry extra information about orientation and need change the rendering part.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Is the camera preview window getting the image from the camera directly instead of actually displaying the gUM stream? I can't explain why you would have this problem otherwise.

Also, I think the patch shouldn't depend on establishing a PeerConnection before determining the rotation to apply, you should do it as soon as gUM happens.

Comment 14

[Eric Rescorla (:ekr)]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #13)
> Is the camera preview window getting the image from the camera directly
> instead of actually displaying the gUM stream? I can't explain why you would
> have this problem otherwise.

As far as I know, the preview window for all the existing calling
apps is the second type.


> Also, I think the patch shouldn't depend on establishing a PeerConnection
> before determining the rotation to apply, you should do it as soon as gUM
> happens.

Agreed.

Comment 15

[Jason Smith [:jsmith]]

We either need to fix this or turn off the feature, so I'm putting this on the nom queue.

Comment 16

[StevenLee[:slee]]

Attachment: rotate video frame in gUM

Hi Ekr,

Could you test the patch on reference phone?

Comment 17

[StevenLee[:slee]]

Attachment: rotate video frame in gUM

Fix the size keeps changing problem.

Comment 18

[StevenLee[:slee]]

(In reply to StevenLee[:slee] from comment #17)
> Created attachment 8377480 [details] [diff] [review]
> rotate video frame in gUM
> 
> Fix the size keeps changing problem.
This patch is for MWC. This version needs one more memcpy(one in MediaEngineWebRTCVideoSource and one in VideoConduit). For original version, we has only one memcpy that is in VideoConduit.

I think we should make GrallocImage(which is callbacked by GonkCamera) carries information about rotation information. When compositor handles the image, it can rotate correctly. For encoder, we should pass the camera mount information to VideoConduit and it does the proper rotation.

Comment 19

[Eric Rescorla (:ekr)]

(In reply to StevenLee[:slee] from comment #16)
> Created attachment 8377443 [details] [diff] [review]
> rotate video frame in gUM
> 
> Hi Ekr,
> 
> Could you test the patch on reference phone?

Negative. IT took away my reference phone.

Comment 20

[Jason Smith [:jsmith]]

(In reply to Eric Rescorla (:ekr) from comment #19)
> (In reply to StevenLee[:slee] from comment #16)
> > Created attachment 8377443 [details] [diff] [review]
> > rotate video frame in gUM
> > 
> > Hi Ekr,
> > 
> > Could you test the patch on reference phone?
> 
> Negative. IT took away my reference phone.

If need be, you could borrow my reference phone to test this with a custom build.

Comment 21

[u459114]

Steven,
Implementation of "rotate video frame in gUM".patch is more self constrain, even though it takes one more memory copy. At this stage, I think we should refurbish this patch and land it in 1.4. For another approach, it doesn't introduce one more memory copy but need to change many class in webrtc encode and playback pipeline, I suggest create a follow up bug to do it.

Comment 22

[StevenLee[:slee]]

Attachment: patch

Hi jesup,

Please help review this patch. 
Thanks

Comment 23

[u459114]

Comment on attachment 8378857 [details] [diff] [review]
patch

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +580,5 @@
> +  layers::GrallocImage *nativeImage = static_cast<layers::GrallocImage*>(aImage);
> +  layers::SurfaceDescriptor handle = nativeImage->GetSurfaceDescriptor();
> +  layers::SurfaceDescriptorGralloc grallocHandle = handle.get_SurfaceDescriptorGralloc();
> +  android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +  void *pMem;

= nullptr;

@@ +582,5 @@
> +  layers::SurfaceDescriptorGralloc grallocHandle = handle.get_SurfaceDescriptorGralloc();
> +  android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +  void *pMem;
> +  uint32_t size = mWidth * mHeight * 3 >> 1;
> +

Move to #603

@@ +648,5 @@
> +  if (mSensorAngle == 0) {
> +    mImage = aImage;
> +  } else {
> +    RotateImage(aImage);
> +  }

mImage = (mSensorAngle == 0) ? aImage : RotateImage(aImage);

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8378857 [details] [diff] [review]
patch

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

r+ with minor nits

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +581,5 @@
> +  layers::SurfaceDescriptor handle = nativeImage->GetSurfaceDescriptor();
> +  layers::SurfaceDescriptorGralloc grallocHandle = handle.get_SurfaceDescriptorGralloc();
> +  android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +  void *pMem;
> +  uint32_t size = mWidth * mHeight * 3 >> 1;

parentheses around the amount to be shifted!  Always do this with shifts and bitwise ops to avoid being burned on order-of-operations (and to avoid confusion).

(mWidth * mHeight * 3) >> 1;

And do you know/believe the compiler isn't smart enough to generate the exact same code from
(mWidth * mHeight * 3) / 2;
?

@@ +617,5 @@
> +  layers::PlanarYCbCrData data;
> +  data.mYChannel = dstPtr;
> +  data.mYSize = IntSize(dstWidth, dstHeight);
> +  data.mYStride = dstWidth * lumaBpp >> 3;
> +  data.mCbCrStride = dstWidth * chromaBpp >> 3;

Again: parentheses, and will the compiler not convert /8 to >>3 here?

@@ +619,5 @@
> +  data.mYSize = IntSize(dstWidth, dstHeight);
> +  data.mYStride = dstWidth * lumaBpp >> 3;
> +  data.mCbCrStride = dstWidth * chromaBpp >> 3;
> +  data.mCbChannel = dstPtr + dstHeight * data.mYStride;
> +  data.mCrChannel = data.mCbChannel +( dstHeight * data.mCbCrStride >> 1);

space after +
parentheses, and same comment about / vs >>

@@ +627,5 @@
> +  data.mPicSize = IntSize(dstWidth, dstHeight);
> +  data.mStereoMode = StereoMode::MONO;
> +
> +  videoImage->SetDataNoCopy(data);
> +  // implicitly releases last image

blank line between these (very minor nit)

Comment 25

[u459114]

Comment on attachment 8378857 [details] [diff] [review]
patch

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +519,5 @@
>    config.mPreviewSize.width = aCapability.width;
>    config.mPreviewSize.height = aCapability.height;
>    mCameraControl->SetConfiguration(config);
>    mCameraControl->Set(CAMERA_PARAM_PICTURESIZE, config.mPreviewSize);
> +  mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, mSensorAngle);

MOZ_ASSERT(mSensorAngle >= 0 && mSensorAngle < 360);

@@ +609,5 @@
> +                        mWidth, mHeight,
> +                        mWidth, mHeight,
> +                        static_cast<libyuv::RotationMode>(mSensorAngle),
> +                        libyuv::FOURCC_NV21);
> +

Unlock graphic buffer here.
graphicBuffer->unlock();

Comment 26

[StevenLee[:slee]]

Attachment: patch -v2

address comment 23 ~ 25.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Removing c-n per convo w/ mreavy.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=bf9c497b57d9

Comment 29

[Maire Reavy [:mreavy]]

Try looks good.  I've asked Nils to start testing the fix.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/38079f225606

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/38079f225606

Comment 32

[u459114]

This is a problem here.

Make a rotation base on camera angle is not totally right.
  mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, mSensorAngle);
This value is a fixed value. On all devices we have on hand, this value(mSensorAngle) is 90 degree

You should also detect the screen orientation as well, compare Screen angle with Sensor angle, then you can make a decision how many degree that we need to rotate in MediaEngineWebRTCVideoSource

For exp
In portrait mode, screen rotation is either 0 or 180(up side down)
 screen angle 0 and sensor angle 90 -> rotate -90 degree(270 degree)
 screen angle 180 and sensor angle 90 -> rotate 90 degree

In landscape mode, screen rotation is either 90 or 270
 screen angle 90 and sensor angle 90 -> keep still, no rotation
 screen angle 270 and sensor angle 90 -> rotate 180 degree

Comment 33

[Jason Smith [:jsmith]]

C.J. - Should we backout based on your comment? Sounds like you are saying this doesn't work correctly.

Comment 34

[Maire Reavy [:mreavy]]

I think it's premature to discuss backing it out.  Let's do a full analysis before deciding on the best path forward.

Comment 35

[Maire Reavy [:mreavy]]

Steven and Randell are discussing this now (in #media).

Comment 36

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #34)
> I think it's premature to discuss backing it out.  Let's do a full analysis
> before deciding on the best path forward.

Last time I checked, the analysis here was supposed to happen before landing, not after. So I fail to see why this was landed on m-c.

Comment 37

[Maire Reavy [:mreavy]]

(In reply to Jason Smith [:jsmith] from comment #36)
> (In reply to Maire Reavy [:mreavy] from comment #34)
> > I think it's premature to discuss backing it out.  Let's do a full analysis
> > before deciding on the best path forward.
> 
> Last time I checked, the analysis here was supposed to happen before
> landing, not after. So I fail to see why this was landed on m-c.

You're talking about the review (analysis) of the patch.  I'm talking about the analysis of CJ's concerns.

3 senior developers reviewed (analyzed) the patch. What was missed in review was subtle.  Kudos to CJ for rechecking and flagging this.  The patch appears to not be wrong, just missing an additional piece of code.  I'm suggesting we wait for the analysis of CJ's concerns (which is happening now) before taking action.

Comment 38

[u459114]

(In reply to Jason Smith [:jsmith] from comment #33)
> C.J. - Should we backout based on your comment? Sounds like you are saying
> this doesn't work correctly.

I don't think we need to backout solution.
Before this patch, you get thing wrong in portrait mode; after this patch, you get thing wrong in landscape mode. None of them are perfect. We are changing logic right now. I am currently working with Steven on this. Please wait for a while.

Comment 39

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #37)
> (In reply to Jason Smith [:jsmith] from comment #36)
> > (In reply to Maire Reavy [:mreavy] from comment #34)
> > > I think it's premature to discuss backing it out.  Let's do a full analysis
> > > before deciding on the best path forward.
> > 
> > Last time I checked, the analysis here was supposed to happen before
> > landing, not after. So I fail to see why this was landed on m-c.
> 
> You're talking about the review (analysis) of the patch.  I'm talking about
> the analysis of CJ's concerns.
> 
> 3 senior developers reviewed (analyzed) the patch. What was missed in review
> was subtle.  Kudos to CJ for rechecking and flagging this.  The patch
> appears to not be wrong, just missing an additional piece of code.  I'm
> suggesting we wait for the analysis of CJ's concerns (which is happening
> now) before taking action.

Actually, what I was talking about was the fact that this wasn't manually tested before this landed by Nils or I because if it was, we would have the caught the bug comment 38 and advised against a landing. There was agreement offline that this was a requirement in order to land this. And there's no unit tests on the patch to boot, which was also a requirement.

Comment 40

[Maire Reavy [:mreavy]]

(In reply to Jason Smith [:jsmith] from comment #39)
> (In reply to Maire Reavy [:mreavy] from comment #37)
> > (In reply to Jason Smith [:jsmith] from comment #36)
> > > (In reply to Maire Reavy [:mreavy] from comment #34)
> > > > I think it's premature to discuss backing it out.  Let's do a full analysis
> > > > before deciding on the best path forward.
> > > 
> > > Last time I checked, the analysis here was supposed to happen before
> > > landing, not after. So I fail to see why this was landed on m-c.
> > 
> > You're talking about the review (analysis) of the patch.  I'm talking about
> > the analysis of CJ's concerns.
> > 
> > 3 senior developers reviewed (analyzed) the patch. What was missed in review
> > was subtle.  Kudos to CJ for rechecking and flagging this.  The patch
> > appears to not be wrong, just missing an additional piece of code.  I'm
> > suggesting we wait for the analysis of CJ's concerns (which is happening
> > now) before taking action.
> 
> Actually, what I was talking about was the fact that this wasn't manually
> tested before this landed by Nils or I because if it was, we would have the
> caught the bug comment 38 and advised against a landing. There was agreement
> offline that this was a requirement in order to land this. And there's no
> unit tests on the patch to boot, which was also a requirement.

The reviewed patch was ready well before Monday morning (which was a key component of the original agreement).  The tree was still open for all checkins early this morning (when the patch was ready).  I saw no reason to treat this bug as different from all the other bugs that were being checked in earlier today.  However, we didn't just check it in.  I asked Ryan to take checkin-needed off the bug and to push a TRY while I updated the email thread you're referring to with this new info and my analysis.  I said on the thread that I was planning to ask for checkin if the TRY was green.  After almost 6 hours, there was no objection on the thread and the TRY was green, so I re-added checkin-needed and updated the thread again to keep everyone in the loop.

Regarding the unit test: The best way to test this functionality is on a real device.  We don't have a unit test for this in Android.  After Tony and I finished our email exchange late last night, I reviewed the options for a unit test, and they didn't seem practical. We don't have unit tests for every functionality;  typically adding unit tests is left up to the discretion of the engineering team, and I didn't feel the need to ask for permission to make an  engineering call.

I've asked Steven and Jesup to investigate if we can tell the emulator to "rotate" in mochitests.  If we can, we can test if the resultant gUM video changes aspect ratio as expected.

Comment 41

[StevenLee[:slee]]

Attachment: rotate video with screen

This patch gets the screen orientation information when starting camera. The rotation degree will be calculated by camera mount angle and the screen orientation. This patch assumes that the camera is back-facing. I will renew a patch later with concerning the front/back camera.

Comment 42

[u459114]

Comment on attachment 8379627 [details] [diff] [review]
rotate video with screen

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

Thanks, please remove debug code and push try.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +532,5 @@
> +
> +}
> +
> +int
> +GetAngle(ScreenOrientation aScreen, int aCamaera) {

EvaluateRotateAngle(ScreenOrientation aScreenAngle, int aCameraAngle)

Comment 43

[u459114]

Comment on attachment 8379627 [details] [diff] [review]
rotate video with screen

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

more debug time assertion

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +546,5 @@
> +      screenAngle = 90;
> +      break;
> +   case eScreenOrientation_LandscapeSecondary:
> +      screenAngle = 270;
> +      break;

default:
  MOZ_ASSERT(false);
  break;

@@ +595,5 @@
>      }
>    } else {
> +    int cameraMount;
> +    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, cameraMount);
> +    MOZ_ASSERT(cameraMount >= 0 && cameraMount < 360);

MOZ_ASSERT(cameraMount == 0 || cameraMount == 90 || cameraMount == 180 || cameraMount == 270);

Comment 44

[StevenLee[:slee]]

Attachment: rotate video with screen

address comment 42 and 43

Comment 45

[StevenLee[:slee]]

(In reply to StevenLee[:slee] from comment #44)
> Created attachment 8379640 [details] [diff] [review]
> rotate video with screen
> 
> address comment 42 and 43
This version rotate the video with concerning of front/back camera information which algorithm is from bug 840244.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8379640 [details] [diff] [review]
rotate video with screen

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

r+ with performance nit to use the observer instead of fetching phone orientation on each frame

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +528,5 @@
>  }
>  
>  void
> +MediaEngineWebRTCVideoSource::Notify(const hal::ScreenConfiguration& aConfiguration) {
> +}

So we need to register an Observer for ScreenConfiguration if we're just reading it directly on every frame?  However, as it takes a bit of work to calculate this, we should do 
mOrientation = aConfiguration.orientation();
and then use that on each frame.

@@ +605,5 @@
>        mCallbackMonitor.Notify();
>      }
>    } else {
> +    int cameraMount;
> +    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, cameraMount);

This value should be read where it was before, when the MediaEngineWebRTCVideoSource is initialized, since the hardware angle of a camera never changes.  However, it's also a fairly low overhead call, so this is at most a minor performance nit, and it avoids adding a member var to cache it locally (and keeps the relevant code together).

@@ +610,5 @@
> +    MOZ_ASSERT(cameraMount == 0 || cameraMount == 90 || cameraMount == 180
> +               || cameraMount == 270);
> +    hal::ScreenConfiguration aConfig;
> +    hal::GetCurrentScreenConfiguration(&aConfig);
> +    mRotation = GetRotateAmount(aConfig.orientation(), cameraMount, mCaptureIndex);

mRotation = GetRotateAmount(mOrientation, cameraMount, mCaptureIndex); and get rid of fetching the config here.

Comment 47

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8379640 [details] [diff] [review]
rotate video with screen

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +560,5 @@
> +    result = (aCamera - screenAngle + 360) % 360;
> +  } else {
> +    //front camera
> +    result = (aCamera + screenAngle) % 360;
> +    result = (360 - result) % 360;

This was in the original Android patch for bug 840244, but it's not in the current code in mozilla-central. The original Android patch also modified video_capture_android.cc/WebRtc_Word32 VideoCaptureAndroid::Start to unconditionally (front/back) reverse this code. I got rid of it in some later bug I can't find right now.

I think your device doesn't have a front camera so you cant' test this now? If so, get rid of it, it's probably wrong.

Current m-c looks like this:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/android/java/src/org/webrtc/videoengine/VideoCaptureAndroid.java?from=VideoCaptureAndroid.java&case=true#231

Comment 48

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #46)
> Comment on attachment 8379640 [details] [diff] [review]
> rotate video with screen
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +528,5 @@
> >  }
> >  
> >  void
> > +MediaEngineWebRTCVideoSource::Notify(const hal::ScreenConfiguration& aConfiguration) {
> > +}
> 
> So we need to register an Observer for ScreenConfiguration if we're just
> reading it directly on every frame?  However, as it takes a bit of work to
> calculate this, we should do 
> mOrientation = aConfiguration.orientation();
> and then use that on each frame.
If content process does not register ScreenConfiguration obserever, only the first GetCurrentScreenConfiguration gets the correct data. So that if the process calls gUM more than once, it cannot get the latest screen state. In the current patch, it gets the screen orientation only once when gUM is trying to start the camera.

> 
> @@ +605,5 @@
> >        mCallbackMonitor.Notify();
> >      }
> >    } else {
> > +    int cameraMount;
> > +    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, cameraMount);
> 
> This value should be read where it was before, when the
> MediaEngineWebRTCVideoSource is initialized, since the hardware angle of a
> camera never changes.  However, it's also a fairly low overhead call, so
> this is at most a minor performance nit, and it avoids adding a member var
> to cache it locally (and keeps the relevant code together).
This is callbacked when camera initialization is done. Because of the camera refactor in these days, the start function of camera [1] is ran on another thread. So that I move the getter function.
> 
> @@ +610,5 @@
> > +    MOZ_ASSERT(cameraMount == 0 || cameraMount == 90 || cameraMount == 180
> > +               || cameraMount == 270);
> > +    hal::ScreenConfiguration aConfig;
> > +    hal::GetCurrentScreenConfiguration(&aConfig);
> > +    mRotation = GetRotateAmount(aConfig.orientation(), cameraMount, mCaptureIndex);
> 
> mRotation = GetRotateAmount(mOrientation, cameraMount, mCaptureIndex); and
> get rid of fetching the config here.
This function is only called when gUM tries to start the camera.

Comment 49

[StevenLee[:slee]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #47)
> I think your device doesn't have a front camera so you cant' test this now?
> If so, get rid of it, it's probably wrong.
Yes, I don't have a front camera on my device. And current B2G cannot select front/back camera until bug 898949 land. 

Randell,
Should I remove it and add assertion when it goes the front camera path?

Comment 50

[Gian-Carlo Pascutto [:gcp]]

This is the Android bug that got rid of that line:
https://bugzilla.mozilla.org/show_bug.cgi?id=862808

Comment 51

[StevenLee[:slee]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #50)
> This is the Android bug that got rid of that line:
> https://bugzilla.mozilla.org/show_bug.cgi?id=862808
Got it, I will update my patch.
Thanks

Comment 52

[StevenLee[:slee]]

Attachment: rotate video with screen - v2

address comment 50.

Push to try, https://tbpl.mozilla.org/?tree=Try&rev=850b6f5800be

Comment 53

[u459114]

(In reply to StevenLee[:slee] from comment #48)
> (In reply to Randell Jesup [:jesup] from comment #46)
> > Comment on attachment 8379640 [details] [diff] [review]
> > rotate video with screen
> > ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> > @@ +528,5 @@
> > >  }
> > >  
> > >  void
> > > +MediaEngineWebRTCVideoSource::Notify(const hal::ScreenConfiguration& aConfiguration) {
> > > +}
> > 
> > So we need to register an Observer for ScreenConfiguration if we're just
> > reading it directly on every frame?  However, as it takes a bit of work to
> > calculate this, we should do 
> > mOrientation = aConfiguration.orientation();
> > and then use that on each frame.
> If content process does not register ScreenConfiguration obserever, only the
> first GetCurrentScreenConfiguration gets the correct data. So that if the
> process calls gUM more than once, it cannot get the latest screen state. In
> the current patch, it gets the screen orientation only once when gUM is
> trying to start the camera.
1. Without observer, sScreenConfigurationObservers in Hal.cpp won't enable update.
http://dxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#196
That's a reason why we need to do registration. 
2. We can update rotation angle in MediaEngineWebRTCVideoSource::Notify, but we should not, unless we are sure media stream listener can handle video frame resolution change in one session. 
Video encoding pipeline in webrtc and media recorder may not able to handle resolution change after recording session started.

Comment 54

[u459114]

(In reply to StevenLee[:slee] from comment #48)
> (In reply to Randell Jesup [:jesup] from comment #46)
> > @@ +605,5 @@
> > >        mCallbackMonitor.Notify();
> > >      }
> > >    } else {
> > > +    int cameraMount;
> > > +    mCameraControl->Get(CAMERA_PARAM_SENSORANGLE, cameraMount);
> > 
> > This value should be read where it was before, when the
> > MediaEngineWebRTCVideoSource is initialized, since the hardware angle of a
> > camera never changes.  However, it's also a fairly low overhead call, so
> > this is at most a minor performance nit, and it avoids adding a member var
> > to cache it locally (and keeps the relevant code together).
> This is callbacked when camera initialization is done. Because of the camera
> refactor in these days, the start function of camera [1] is ran on another
> thread. So that I move the getter function.
Steven,
This is terrible. No one knows that he can get correct camera property only after OnHardwareStateChange callback. Let's look into camera implementation next week and file a bug if need.

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

(In reply to C.J. Ku[:CJKu] from comment #53)
> (In reply to StevenLee[:slee] from comment #48)
> > (In reply to Randell Jesup [:jesup] from comment #46)
> > > Comment on attachment 8379640 [details] [diff] [review]
> > > rotate video with screen
> > > ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> > > @@ +528,5 @@
> > > >  }
> > > >  
> > > >  void
> > > > +MediaEngineWebRTCVideoSource::Notify(const hal::ScreenConfiguration& aConfiguration) {
> > > > +}
> > > 
> > > So we need to register an Observer for ScreenConfiguration if we're just
> > > reading it directly on every frame?  However, as it takes a bit of work to
> > > calculate this, we should do 
> > > mOrientation = aConfiguration.orientation();
> > > and then use that on each frame.
> > If content process does not register ScreenConfiguration obserever, only the
> > first GetCurrentScreenConfiguration gets the correct data. So that if the
> > process calls gUM more than once, it cannot get the latest screen state. In
> > the current patch, it gets the screen orientation only once when gUM is
> > trying to start the camera.
> 1. Without observer, sScreenConfigurationObservers in Hal.cpp won't enable
> update.
> http://dxr.mozilla.org/mozilla-central/source/hal/Hal.cpp#196
> That's a reason why we need to do registration. 

Ok, that deserves a comment then.

> 2. We can update rotation angle in MediaEngineWebRTCVideoSource::Notify, but
> we should not, unless we are sure media stream listener can handle video
> frame resolution change in one session. 
> Video encoding pipeline in webrtc and media recorder may not able to handle
> resolution change after recording session started.

gUM does not (and never did) guarantee that frame-to-frame resolutions will not change.  Any consumer of gUM MediaStreams (or any other MediaStreams, not just gUM) must handle this.  PeerConnection does handle it.  So we should update the orientation in Notify().

Comment 56

[u459114]

(In reply to Randell Jesup [:jesup] from comment #55)
> (In reply to C.J. Ku[:CJKu] from comment #53)
> > (In reply to StevenLee[:slee] from comment #48)
> > 2. We can update rotation angle in MediaEngineWebRTCVideoSource::Notify, but
> > we should not, unless we are sure media stream listener can handle video
> > frame resolution change in one session. 
> > Video encoding pipeline in webrtc and media recorder may not able to handle
> > resolution change after recording session started.
> 
> gUM does not (and never did) guarantee that frame-to-frame resolutions will
> not change.  Any consumer of gUM MediaStreams (or any other MediaStreams,
> not just gUM) must handle this.  PeerConnection does handle it.  So we
> should update the orientation in Notify().
Understand, but I would suggest not right now. We need more test before enable per frame orientation change, especially for media recorder module. (bug 891704/ bug 879668 had landed, we can already record a video by standard webapi on B2G). Only detect orientation in the beginning of recording session is safer for now. I think we may file another following bug and enable it after more test, agree?

Comment 57

[cmtalbert]

(In reply to StevenLee[:slee] from comment #52)
> Created attachment 8379738 [details] [diff] [review]
> rotate video with screen - v2
> 
> address comment 50.
> 
> Push to try, https://tbpl.mozilla.org/?tree=Try&rev=850b6f5800be

There is a mochitest crash on b2g emulators (m1) and a getUserMedia test timeout also on b2g emulators (m10). I've retriggered those jobs to see if those are intermittent issues or if they are constant (if constant, they could be a result of this patch).

Comment 58

[cmtalbert]

(In reply to Clint Talbert ( :ctalbert ) from comment #57)
> (In reply to StevenLee[:slee] from comment #52)
> > Created attachment 8379738 [details] [diff] [review]
> > rotate video with screen - v2
> > 
> > address comment 50.
> > 
> > Push to try, https://tbpl.mozilla.org/?tree=Try&rev=850b6f5800be
> 
> There is a mochitest crash on b2g emulators (m1) and a getUserMedia test
> timeout also on b2g emulators (m10). I've retriggered those jobs to see if
> those are intermittent issues or if they are constant (if constant, they
> could be a result of this patch).

Ok, Looks like those are constant crashes/test failures. They happened on each re-trigger I ran. Steven Lee, I think you will need to review the errors with an eye to what you've changed in this patch, please.

Comment 59

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Clint Talbert ( :ctalbert ) from comment #57)
> (In reply to StevenLee[:slee] from comment #52)
> > Created attachment 8379738 [details] [diff] [review]
> > rotate video with screen - v2
> > 
> > address comment 50.
> > 
> > Push to try, https://tbpl.mozilla.org/?tree=Try&rev=850b6f5800be
> 
> There is a mochitest crash on b2g emulators (m1) and a getUserMedia test
> timeout also on b2g emulators (m10). I've retriggered those jobs to see if
> those are intermittent issues or if they are constant (if constant, they
> could be a result of this patch).

EnumerateVideoDevices() (and the VideoSource Init() call) runs on the MediaManager thread, not mainthread.

I have a try running that moves these to already-called-on-MainThread functions:
https://tbpl.mozilla.org/?tree=Try&rev=bed0d77056dd

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Randell Jesup [:jesup] from comment #59)
> (In reply to Clint Talbert ( :ctalbert ) from comment #57)

> EnumerateVideoDevices() (and the VideoSource Init() call) runs on the
> MediaManager thread, not mainthread.
> 
> I have a try running that moves these to already-called-on-MainThread
> functions:
> https://tbpl.mozilla.org/?tree=Try&rev=bed0d77056dd

This Try is green on the two tests that had problems before.  I'll upload the patch.

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Register observers for ScreenConfiguration from MainThread

Comment 62

[Jason Smith [:jsmith]]

First, I'm going to reopen this as we're working on immediate followup here.

Also - Can we please manually verify the patch before we land this again? I know for a fact that the original patch didn't work here, as the camera orientation still isn't in alignment with the phone's orientation on today's build.

Comment 63

[Maire Reavy [:mreavy]]

(In reply to Jason Smith [:jsmith] from comment #62)
> First, I'm going to reopen this as we're working on immediate followup here.
> 
> Also - Can we please manually verify the patch before we land this again? I
> know for a fact that the original patch didn't work here, as the camera
> orientation still isn't in alignment with the phone's orientation on today's
> build.

Yes, I thought the patch had been manually verified when I asked for checkin-needed on the first patch.  I'm tracking the testing/verification much more closely until this issue fully resolves.

Thanks for reopening.

Comment 64

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #59)
> EnumerateVideoDevices() (and the VideoSource Init() call) runs on the
> MediaManager thread, not mainthread.
> 
> I have a try running that moves these to already-called-on-MainThread
> functions:
> https://tbpl.mozilla.org/?tree=Try&rev=bed0d77056dd
I think that should be the cause. I am building debug version and trying jesup's patch.

Comment 65

[Randell Jesup [:jesup] (needinfo me)]

Attachment: bug970183_wip

WIP (applies on top of Steven's patch) that fixes rotation/aspect ratio, and also supports dynamic rotation in mid-capture.  One known bug (transfer to 0 rotate, then to non-0 crashes)

Comment 66

[Randell Jesup [:jesup] (needinfo me)]

Attachment: rotation_complete

Works with all rotations; there's a crash in layers if I change the format of an image from one frame to the next, so we convert to I420 for all rotations.

Comment 67

[Eric Rescorla (:ekr)]

Attachment: 0001-Fix-jesup-s-patch-for-back-camera-on-Flame.patch

Unfortunately, Jesup's patch (rotation_complete) does not appear to work
properly on Flame's back camera, partly because 0 appears to be the front
for Flame.

The following hacky patch works properly with both Flame cameras but messes up Helix

Comment 68

[StevenLee[:slee]]

Attachment: get camera front/back information

Get the camera front/back information through ICameraControl::GetCameraName

Comment 69

[StevenLee[:slee]]

Attachment: Part 2: get camera front/back information

Fix dumb problem. When calling GetRotateAmount, it should pass back/front camera information rather than camera index.

Comment 70

[u459114]

Comment on attachment 8380193 [details] [diff] [review]
Part 2: get camera front/back information

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +625,5 @@
> +    ICameraControl::GetCameraName(mCaptureIndex, deviceName);
> +    if (deviceName.EqualsASCII("back")) {
> +      mBackCamera = true;
> +    }
> +

LowerCaseEqualsASCII?

Comment 71

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8380131 [details] [diff] [review]
rotation_complete

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

ok, putting up rolled-up patch for review (for my changes to Steven's patch) to CJ/Steven.  I'll also upload an interdiff patch just showing what I changed.

Main change from Steven's patch:  a) move register/unregister calls to mainthread functions, b) modify mWidth/mHeight handling, c) optional support for changing rotation in Notify() (which we'll disable for now until MediaRecorder can handle it), d) cache cameraAngle, e) always convert to I420 to avoid gfx bug in UpdateImage (In theory we could leave this out if we don't support dynamic rotation, but it only helps in one orientation anyways, and we'll need to deal with it soon anyways.)

Note: this doesn't include Steven's front/back patch, which I and CJ will review.

Comment 72

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8380193 [details] [diff] [review]
Part 2: get camera front/back information

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

r+ - since the ascii tags are hard-defined in http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraManager.cpp#34 I don't think we need to do lowercase.  (Better would be for them to be in an include file as defines, but that's not needed)

Comment 73

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8380193 [details] [diff] [review]
Part 2: get camera front/back information

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +572,1 @@
>    LOG(("*** New orientation: %d (Camera %d MountAngle: %d)", mRotation, mCaptureIndex, mCameraAngle));

please update the LOG() messages to indicate if it's a Back camera as well

@@ +631,1 @@
>      LOG(("*** Initial orientation: %d (Camera %d MountAngle: %d)", mRotation, mCaptureIndex, mCameraAngle));

Update LOG() to indicate back camera as well

Comment 74

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Interdiffs for jesup's mods to stevenlee's patch (for ease in reviewing)

Comment 75

[StevenLee[:slee]]

Comment on attachment 8380131 [details] [diff] [review]
rotation_complete

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

rjesup, 
Thanks for your help.
Here is the try server log. 
https://tbpl.mozilla.org/?tree=Try&rev=0ecedc4a4065

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +561,5 @@
> +  return result;
> +}
> +
> +// undefine to remove on-the-fly rotation support
> +#define DYNAMIC_GUM_ROTATION

We should remove this before encoder can handle it, right?

Comment 76

[StevenLee[:slee]]

Comment on attachment 8380131 [details] [diff] [review]
rotation_complete

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

Sorry, I selected the wrong flag.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +561,5 @@
> +  return result;
> +}
> +
> +// undefine to remove on-the-fly rotation support
> +#define DYNAMIC_GUM_ROTATION

We should remove this before encoder can handle it, right?

Comment 77

[Jason Smith [:jsmith]]

Going to clear blocking flag as gUM doesn't hit the QC feature list & DSDS feature list

Comment 78

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8380193 [details] [diff] [review]
Part 2: get camera front/back information

Looks good to me.

Comment 79

[Randell Jesup [:jesup] (needinfo me)]

Summary of discussion with mwu and mikeh in #b2g:
Some devices don't have correct camera angle settings.  Unagi and Helix and Sora are correct (and I think Buri).  Current patchset has been verified on Unagi and Helix so far.  Flame likely is not (currently).

https://tbpl.mozilla.org/?tree=Try&rev=bdcc1b1fbcb9
(Note that this was on top of nfroyd's patch that messes up gUM success in debug only; thus the M1 failure in emulator debug.)

Comment 80

[Maire Reavy [:mreavy]]

We have two solid r+'d patches that I've seen personally tested on a Helix this weekend.  Steven has verified they work on the Unagi.

The fix is busted on Flame because (per mwu) the manufacturer is misreporting the sensor angles, and the manufacturer has to fix it; this device bug has also busted the camera app on the Flame.

If QA (or someone with QA's proxy) can verify mwu's information is correct, I'd like permission to land this given the known Flame manufacturer bug (once the fix has been verified by Nils or someone else from QA on a known working device).

Note: We can pull together a patch today to kludge around the manufacturer's bug, but if the bug is going to be fixed before the Flame goes to production (which would be a hard requirement), putting in a temporary kludge seems unnecessary, and we'd need to pull it when the manufacturer fixes the bug.

Comment 81

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #80)
> We have two solid r+'d patches that I've seen personally tested on a Helix
> this weekend.  Steven has verified they work on the Unagi.
> 
> The fix is busted on Flame because (per mwu) the manufacturer is
> misreporting the sensor angles, and the manufacturer has to fix it; this
> device bug has also busted the camera app on the Flame.

Can we get a vendcom bug on file for this? Vance probably wants to know about this.

> 
> If QA (or someone with QA's proxy) can verify mwu's information is correct,
> I'd like permission to land this given the known Flame manufacturer bug
> (once the fix has been verified by Nils or someone else from QA on a known
> working device).

That's fine. FWIW - I'm not surprised there's a problem with the Flame here, as we're finding a bunch of vendor problems with that device.

> 
> Note: We can pull together a patch today to kludge around the manufacturer's
> bug, but if the bug is going to be fixed before the Flame goes to production
> (which would be a hard requirement), putting in a temporary kludge seems
> unnecessary, and we'd need to pull it when the manufacturer fixes the bug.

Don't worry about it. Let's get the vendor to fix the problem here.

Comment 82

[Jason Smith [:jsmith]]

Nils is currently flashing a custom build to test this.

Comment 83

[Nils Ohlmeier [:drno]]

I ran on my Helix device a local build with today's m-c plus the two r+ patches from this ticket and the patches from 898949.
Video in a WebRTC call from the front facing camera on the Helix rotates properly as expected.
Video in a WebRTC call from the back camera rotates as well. A little unexpected is that in this case the video is already 90 rotated when you hold the device in a 45 degree angle. By the time you rotate the device 90 degree the local picture is already rotated 180 degree. But when this patch kicks in the local picture get reset to the expected rotation matching how the device is being held.
Camera app still works as expected with both cameras.

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

The issues you mention are to be expected due to the geometry of the cameras and front vs back positions.

Note there's a bug in FxOS (mwu has a patch) where a quick 180 deg rotate isn't noticed or isn't noticed for a while (or until you jostle it enough).  That affects the entire phone, so it's not something we need to be concerned with here if you see it.  mwu's patch causes orange, so hasn't landed yet.

Comment 85

[Maire Reavy [:mreavy]]

Jason -- Nils has confirmed that the patch works on a Helix.  Are there other devices that need to be tested/verified first, or can we mark this checkin-needed now?   Thanks.

Comment 86

[Jason Smith [:jsmith]]

(In reply to Maire Reavy [:mreavy] from comment #85)
> Jason -- Nils has confirmed that the patch works on a Helix.  Are there
> other devices that need to be tested/verified first, or can we mark this
> checkin-needed now?   Thanks.

I checked on Buri as well - this looks good to me.

I realized that the perms check doesn't need to happen on this bug - that needs to happen instead on the front/back camera bug. I'll do separately, but that testing doesn't need to block landing this.

Comment 87

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Part 1: Support phone rotation in gUM at start of capture

patch for checkin, which also disables dynamic rotation per the discussion (until MediaRecorder can handle it).

Comment 88

[Randell Jesup [:jesup] (needinfo me)]

Note: only parts 1 & 2 should land, not the Flame patch

Comment 89

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/b2g-inbound/rev/edb399194be9

Comment 90

[Carsten Book [:Tomcat]]

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

Comment 91

[Peter Chang[:pchang]]

create bug 976397 to rotate camera preview by changing the transform of image layer to save the memory copy.