Closed Bug 970183 Opened 6 years ago Closed 6 years ago

[B2G getUserMedia] Camera always callback landscape frames

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: slee, Assigned: slee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ft:webrtc])

Attachments

(3 files, 13 obsolete files)

1.88 KB, patch
Details | Diff | Splinter Review
4.17 KB, patch
jesup
: review+
u459114
: review+
mikeh
: feedback+
Details | Diff | Splinter Review
11.06 KB, patch
jesup
: review+
Details | Diff | Splinter Review
gUM should listen to orientation sensor and tell camera to callback correct video frames.
Whiteboard: [mwcdemo2014]
Assignee: nobody → slee
(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.
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.
(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.
Blocks: 923361
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
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
(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
Attached patch rotate_video - WIP (obsolete) — Splinter Review
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.
Stephen,

The frames are also rotated in the preview window.

It seems like we want to do the derotation on the gUM side.
(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.
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.
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.
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.
(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.
We either need to fix this or turn off the feature, so I'm putting this on the nom queue.
blocking-b2g: --- → 1.4?
Target Milestone: --- → 1.4 S2 (28feb)
Whiteboard: [mwcdemo2014] → [mwcdemo2014][ft:webrtc]
Attached patch rotate video frame in gUM (obsolete) — Splinter Review
Hi Ekr,

Could you test the patch on reference phone?
Attached patch rotate video frame in gUM (obsolete) — Splinter Review
Fix the size keeps changing problem.
Attachment #8377443 - Attachment is obsolete: true
(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.
(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.
(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.
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.
Attached patch patch (obsolete) — Splinter Review
Hi jesup,

Please help review this patch. 
Thanks
Attachment #8377480 - Attachment is obsolete: true
Attachment #8378857 - Flags: review?(rjesup)
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 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)
Attachment #8378857 - Flags: review?(rjesup) → review+
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();
Attached patch patch -v2 (obsolete) — Splinter Review
address comment 23 ~ 25.
Attachment #8378857 - Attachment is obsolete: true
Attachment #8378924 - Flags: review+
Keywords: checkin-needed
Removing c-n per convo w/ mreavy.
Keywords: checkin-needed
Attachment #8376170 - Attachment is obsolete: true
Try looks good.  I've asked Nils to start testing the fix.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38079f225606
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
C.J. - Should we backout based on your comment? Sounds like you are saying this doesn't work correctly.
Flags: needinfo?(cku)
I think it's premature to discuss backing it out.  Let's do a full analysis before deciding on the best path forward.
Steven and Randell are discussing this now (in #media).
(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.
(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.
(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.
Flags: needinfo?(cku)
(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.
(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.
Attached patch rotate video with screen (obsolete) — Splinter Review
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 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 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);
Attached patch rotate video with screen (obsolete) — Splinter Review
address comment 42 and 43
Attachment #8379627 - Attachment is obsolete: true
Attachment #8379640 - Flags: review?(rjesup)
(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.
Depends on: 975359
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.
Attachment #8379640 - Flags: review?(rjesup) → review+
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
(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.
(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?
This is the Android bug that got rid of that line:
https://bugzilla.mozilla.org/show_bug.cgi?id=862808
(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
Attached patch rotate video with screen - v2 (obsolete) — Splinter Review
address comment 50.

Push to try, https://tbpl.mozilla.org/?tree=Try&rev=850b6f5800be
Attachment #8379640 - Attachment is obsolete: true
Attachment #8379738 - Flags: review+
(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.
(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.
(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().
(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?
(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).
(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.
Flags: needinfo?(slee)
(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
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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.
Flags: needinfo?(slee)
Attached patch bug970183_wip (obsolete) — Splinter Review
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)
Attached patch rotation_complete (obsolete) — Splinter Review
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.
Attachment #8380130 - Attachment is obsolete: true
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
Get the camera front/back information through ICameraControl::GetCameraName
Fix dumb problem. When calling GetRotateAmount, it should pass back/front camera information rather than camera index.
Attachment #8380185 - Attachment is obsolete: true
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 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.
Attachment #8380131 - Flags: review?(slee)
Attachment #8380131 - Flags: review?(cku)
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)
Attachment #8380193 - Flags: review+
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
Attachment #8379963 - Attachment is obsolete: true
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?
Attachment #8380131 - Flags: review?(slee) → review-
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?
Attachment #8380131 - Flags: review- → review+
Going to clear blocking flag as gUM doesn't hit the QC feature list & DSDS feature list
blocking-b2g: 1.4? → ---
Attachment #8378924 - Attachment is obsolete: true
Attachment #8379738 - Attachment is obsolete: true
Comment on attachment 8380193 [details] [diff] [review]
Part 2: get camera front/back information

Looks good to me.
Attachment #8380193 - Flags: feedback+
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.)
Attachment #8380131 - Flags: review?(cku) → review+
Attachment #8380193 - Flags: review?(cku) → review+
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.
(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.
Nils is currently flashing a custom build to test this.
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.
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(jsmith)
Keywords: checkin-needed
patch for checkin, which also disables dynamic rotation per the discussion (until MediaRecorder can handle it).
Attachment #8380131 - Attachment is obsolete: true
Attachment #8380210 - Attachment is obsolete: true
Attachment #8381093 - Attachment description: Support phone rotation in gUM at start of capture → Part 1: Support phone rotation in gUM at start of capture
Attachment #8380193 - Attachment description: get camera front/back information → Part 2: get camera front/back information
Note: only parts 1 & 2 should land, not the Flame patch
https://hg.mozilla.org/mozilla-central/rev/edb399194be9
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
create bug 976397 to rotate camera preview by changing the transform of image layer to save the memory copy.
Keywords: verifyme
QA Contact: jsmith
You need to log in before you can comment on or make changes to this bug.