1143694 - WebRTC looks garbled on Lollipop Gonk

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Diego Wilson [:diego]]

Loopback works fine [1].

In an actual apprtc [2] session the outgoing video of the L-gonk phone looks garbled on the remote KK phone side [3]. The incoming video looks fine on the L-gonk phone[4]. I tried this with L-phone to KK-gonk phone and L-gonk phone to Desktop. I got the same result.

Did you get a chance to try a phone to phone session on v2.2 Lollipop?

[1] http://mozilla.github.io/webrtc-landing/pc_test.html
[2] https://apprtc.appspot.com/
[3] https://www.dropbox.com/s/u9uum0yk0gy73zw/2015-03-13%2014.29.27.jpg?dl=0
[4] https://www.dropbox.com/s/umi9zo4kvw7gd8n/2015-03-13%2014.29.32.jpg?dl=0

Comment 1

[Sotaro Ikeda [:sotaro]]

diego, thanks for creating the bug. Do you enable H.264 WebRTC with the following?

> pref("media.peerconnection.video.h264_enabled", true);

Comment 2

[Sotaro Ikeda [:sotaro]]

I did h.264 WebRTC session with [2] between v2.2 nexus-5-l and v2.2 flame-kk, but I did not saw the problem. It might be device dependent problem.

Comment 3

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> diego, thanks for creating the bug. Do you enable H.264 WebRTC with the
> following?
> 
> > pref("media.peerconnection.video.h264_enabled", true);

I can reproduce this issue on my device with either H.264 enabled or disabled.

Comment 4

[Sotaro Ikeda [:sotaro]]

One possibility is the following.
- WebRTC gonk implementation expect YUV420SP as camera preview's color format.
- The device's camera preview might not be YUV420SP and ConvertToI420() failed to convert the preview.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: log patch - Add log around MediaEngineGonkVideoSource

Comment 6

[Sotaro Ikeda [:sotaro]]

diego, can you get a log by using attachment 8578135 [details] [diff] [review] on your v2.2 lollipop device?

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: log patch - Add log around MediaEngineGonkVideoSource

Remove the following from the patch.

> pref("media.peerconnection.video.h264_enabled", true);

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> diego, can you get a log by using attachment 8578135 [details] [diff] [review]
> [review] on your v2.2 lollipop device?

attachment 8578137 [details] [diff] [review] is latest log patch.

Comment 9

[Maire Reavy [:mreavy]]

Hi Sotaro - Can you take this bug?   Thanks.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #9)
> Hi Sotaro - Can you take this bug?   Thanks.

Okey, I could take this bug. But we do not have a hardware and development environment that could reproduce the problem. Therefore, we need help from codeaurora.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Okey, I could take this bug. But we do not have a hardware and development
> environment that could reproduce the problem. Therefore, we need help from
> codeaurora.

because nexus-5-l did not reproduce the problem.

Comment 12

[Diego Wilson [:diego]]

Attachment: Garbled preview logcat

Sotaro,

I added your patches. I see there's some color conversion errors there. I think there's something missing in the newer baseline. I'll look into in further.

Comment 13

[Sotaro Ikeda [:sotaro]]

Okey, thanks.

Comment 14

[Michael Vines [:m1] [:evilmachines]]

(ni? :dwilson based on comment 12)

Comment 15

[Diego Wilson [:diego]]

Turns out the issue is somewhere in webrtc image rotation:

https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#722

On my device it unsuccessfully rotates the camera frames by 270 degrees which garbles the video before encoding. If I force a 0 degree rotation the video looks fine but with the wrong rotation.

Any idea what could be going wrong here?

Comment 16

[Sotaro Ikeda [:sotaro]]

diego, mRotation seems to be calculated by MediaEngineGonkVideoSource::GetRotation(). Can you analyze it on your device?

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> diego, mRotation seems to be calculated by
> MediaEngineGonkVideoSource::GetRotation().

https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#522

Comment 18

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > diego, mRotation seems to be calculated by
> > MediaEngineGonkVideoSource::GetRotation().
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> MediaEngineGonkVideoSource.cpp#522

I believe the rotation value of 270 degrees is correct. The problem is that performing that rotation [1] looks messed up. Perhaps the stride calculations [2] are not taking rotation?

[1] https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#772
[2] https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#770

Comment 19

[Diego Wilson [:diego]]

This seems to be an alignment/rotation issue. If I force the rotated width to be aligned to 16 pixels [1] the same way the stride is being aligned [2] then the video looks ok (except for a green bar filling up the extra alignment padding).

[1] https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#741
[2] https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineGonkVideoSource.cpp#770

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #18)
> (In reply to Sotaro Ikeda [:sotaro] from comment #17)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > > diego, mRotation seems to be calculated by
> > > MediaEngineGonkVideoSource::GetRotation().
> > 
> > https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> > MediaEngineGonkVideoSource.cpp#522
> 
> I believe the rotation value of 270 degrees is correct. The problem is that
> performing that rotation [1] looks messed up. Perhaps the stride
> calculations [2] are not taking rotation?

I just checked flame-kk. Its camera was also 270 degree.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #18)
> (In reply to Sotaro Ikeda [:sotaro] from comment #17)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > > diego, mRotation seems to be calculated by
> > > MediaEngineGonkVideoSource::GetRotation().
> > 
> > https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> > MediaEngineGonkVideoSource.cpp#522
> 
> I believe the rotation value of 270 degrees is correct. The problem is that
> performing that rotation [1] looks messed up. Perhaps the stride
> calculations [2] are not taking rotation?

Destination uses HAL_PIXEL_FORMAT_YV12, its stride is based on the following. The way of the calculation seems correct.

http://androidxref.com/5.1.0_r1/xref/system/core/include/system/graphics.h#92

Comment 22

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> (In reply to Diego Wilson [:diego] from comment #18)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #17)
> > > (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > > > diego, mRotation seems to be calculated by
> > > > MediaEngineGonkVideoSource::GetRotation().
> > > 
> > > https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> > > MediaEngineGonkVideoSource.cpp#522
> > 
> > I believe the rotation value of 270 degrees is correct. The problem is that
> > performing that rotation [1] looks messed up. Perhaps the stride
> > calculations [2] are not taking rotation?
> 
> I just checked flame-kk. Its camera was also 270 degree.

I believe the Flame camera preview defaults to 176x144 which is already 16 pixel aligned.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: log patch - Add log around MediaEngineGonkVideoSource

Add log around stride.

Comment 24

[Sotaro Ikeda [:sotaro]]

Diego, can you take a log by using attachment 8584694 [details] [diff] [review]? I wanted to know concrete stride info. Thanks.

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - use stride as source buffer width

Comment 26

[Sotaro Ikeda [:sotaro]]

Diego, can you test how attachment 8584717 [details] [diff] [review] works?

Comment 27

[Diego Wilson [:diego]]

Sotaro,

It's definitely a stride issue. The webrtc video capture pipeline does not carry over the stride of the camera frames. One solution is to align the webrtc Y stride for YV12 to 16 pixels here:

https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/common_video/libyuv/webrtc_libyuv.cc#248

What do you think?

Comment 28

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

Is the 16-pixel alignment a requirement of YV12 per-se?  If so, that would be a reasonable place to put code to enforce it (or code to complain if it isn't).  However, a more general fix (perhaps additional, since it would cover all formats as needed) would be to do the graphicBuffer->GetStride() calls in the code that calls this.

(Speaking of which: sotaro, shouldn't it use GetStride for both the ConvertToI420() calls in that function?  Other than that question, r+ for the GetStride patch.)

Comment 29

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #27)
> Sotaro,
> 
> It's definitely a stride issue. The webrtc video capture pipeline does not
> carry over the stride of the camera frames. One solution is to align the
> webrtc Y stride for YV12 to 16 pixels here:
> 
> https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> common_video/libyuv/webrtc_libyuv.cc#248
> 
> What do you think?

I also checked the code, therefore, I created attachment 8584717 [details] [diff] [review], could you respond to comment 24 and comment 25?

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Randell Jesup [:jesup] from comment #28)
> Is the 16-pixel alignment a requirement of YV12 per-se? 

I do not think it is a requirement of YV12. But it might happen often because of performance reason.

Comment 31

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> I also checked the code, therefore, I created attachment 8584717 [details] [diff] [review]
> [diff] [review], could you respond to comment 24 and comment 25?

Sotaro,

Your patch is for the non-gralloc case. Camera buffers (on my device at least) are gralloc buffers. When I do an equivalent change for gralloc it doesn't work. It seems the stride and the width need to have different values here as I suggest in comment 27.

And I agree with Randell that ideally we would explicitly carry over the stride but that would require changing the mediapipeline to include stride values in addition to width/height.

Comment 32

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #31)
> (In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > I also checked the code, therefore, I created attachment 8584717 [details] [diff] [review]
> > [diff] [review], could you respond to comment 24 and comment 25?
> 
> Sotaro,
> 
> Your patch is for the non-gralloc case.

Sorry, about it.

> Camera buffers (on my device at
> least) are gralloc buffers. When I do an equivalent change for gralloc it
> doesn't work. It seems the stride and the width need to have different
> values here as I suggest in comment 27.

I assume that the reason it did not work is related to the following code.

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/common_video/libyuv/webrtc_libyuv.cc#

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> I assume that the reason it did not work is related to the following code.
> 
> https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> common_video/libyuv/webrtc_libyuv.cc#

Sorry, like was missing.

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/common_video/libyuv/webrtc_libyuv.cc#245

Comment 34

[Diego Wilson [:diego]]

So what do you think of my suggestion in comment 27? Do you think that solution is too narrow?

Comment 35

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #27)
> Sotaro,
> 
> It's definitely a stride issue. The webrtc video capture pipeline does not
> carry over the stride of the camera frames. One solution is to align the
> webrtc Y stride for YV12 to 16 pixels here:
> 
> https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> common_video/libyuv/webrtc_libyuv.cc#248
> 
> What do you think?

This code is gonk specific. It is not general YV12 code. Then as a simple fix, " Y stride for YV12 to 16 pixels" seems reasonable.

Comment 36

[Sotaro Ikeda [:sotaro]]

But as a long term fix, we need general fix. because it is not a android YV12's requirement.

Comment 37

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> But as a long term fix, we need general fix. because it is not a android
> YV12's requirement.

Sorry, I misunderstood it. From the following, it is a requirement of android's YV12. Therefore, "Y stride for YV12 to 16 pixels" could be long term solution.

http://androidxref.com/5.1.0_r1/xref/system/core/include/system/graphics.h#104

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Care about gralloc YV12 stride

Comment 39

[Sotaro Ikeda [:sotaro]]

Diego, does attachment 8587030 [details] [diff] [review] works on your device?

Comment 40

[Diego Wilson [:diego]]

Comment on attachment 8587030 [details] [diff] [review]
patch - Care about gralloc YV12 stride

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

This patch fixes it for me. Cleared for landing!

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #40)
> 
> This patch fixes it for me. Cleared for landing!

Thanks for the checking!

Comment 42

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=910d79be3dcd

Comment 43

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21e4e3a2b33d

Comment 44

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

https://hg.mozilla.org/mozilla-central/rev/21e4e3a2b33d

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/50848a1a89ae

Comment 46

[Hermes Cheng (inactive after July 27, 2015)]

verify 2.2 with below build

* test env
Build ID               20150406162505
Gaia Revision          485322797a069e5b185931e87d15d4e921e25b64
Gaia Date              2015-04-06 18:36:29
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f766e5052b39
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150406.195612
Firmware Date          Mon Apr  6 19:56:28 EDT 2015
Bootloader             HHZ12d

Comment 47

[Pi Wei Cheng [:piwei] (inactive)]

We don't have the device or build mentioned on comment 46. Removing this bug from our queries.