1066410 - Serious color distortion happened when plays WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Vincent Liu[:vliu]]

* Precondition:
  1. Launch browser.
  2. Visit: http://apprtc.appspot.com/
  3. Check the preview screen

* Expected result:
  Preview screen no color distortion happened

* Actual result:
  Serious color distortion in preview screen.

Comment 1

[Vincent Liu[:vliu]]

This issue happens in partner device. Currently Gecko hard coding the pixel format NV21 when it wants to rotate image for preview flame.

http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#1006

To fit for the variety of different pixel format, I think it should be determined by the value queried from graphicBuffer.

Comment 2

[Vincent Liu[:vliu]]

Attachment: bug-1066410-fix.patch

Hi Randell,

Would you please help me to review the patch? Really thanks.

Comment 3

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

Comment on attachment 8488428 [details] [diff] [review]
bug-1066410-fix.patch

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

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +196,5 @@
>    void StartImpl(webrtc::CaptureCapability aCapability);
>    void StopImpl();
>    void SnapshotImpl();
>    void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight);
> +  uint32_t ConvertPixexFormatToFOURCC(uint32_t aFormat);

See .cpp file

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +968,5 @@
>    }
>  }
>  
> +uint32_t
> +MediaEngineWebRTCVideoSource::ConvertPixexFormatToFOURCC(uint32_t aFormat) {

{ on new line
PixelFormat is the theoretical type here (returned by getPixelFormat()), and they're all defined as that enum or anonymous enums, which means PixelFormat or perhaps 'int' is the correct type for aFormat - though in practice it doesn't matter much until FxOS runs on 64-bit ARM

@@ +975,5 @@
> +    return libyuv::FOURCC_NV21;
> +  case HAL_PIXEL_FORMAT_YV12:
> +    return libyuv::FOURCC_YV12;
> +  default: {
> +    LOG((" xxxxx Unknow pixel format %d\n", aFormat));

"Unknown", and remove \n

Comment 4

[Vincent Liu[:vliu]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8488428 [details] [diff] [review]
> bug-1066410-fix.patch
> 
> Review of attachment 8488428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +196,5 @@
> >    void StartImpl(webrtc::CaptureCapability aCapability);
> >    void StopImpl();
> >    void SnapshotImpl();
> >    void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight);
> > +  uint32_t ConvertPixexFormatToFOURCC(uint32_t aFormat);
> 
> See .cpp file
> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +968,5 @@
> >    }
> >  }
> >  
> > +uint32_t
> > +MediaEngineWebRTCVideoSource::ConvertPixexFormatToFOURCC(uint32_t aFormat) {
> 
> { on new line
> PixelFormat is the theoretical type here (returned by getPixelFormat()), and
> they're all defined as that enum or anonymous enums, which means PixelFormat
> or perhaps 'int' is the correct type for aFormat - though in practice it
> doesn't matter much until FxOS runs on 64-bit ARM
> 

Do you think if it better including android header file and declaring the method below?

In MediaEngineWebRTC.h

namespace android {
#include <ui/PixelFormat.h>
}

class MediaEngineWebRTCVideoSource : public MediaEngineVideoSource
                                   , public nsRunnable
                                   ...
{
   ...
   uint32_t ConvertPixexFormatToFOURCC(android::PixelFormat aFormat);
   ...
};

Comment 5

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

(In reply to Vincent Liu[:vliu] from comment #4)

> Do you think if it better including android header file and declaring the
> method below?
> 
> In MediaEngineWebRTC.h
> 
> namespace android {
> #include <ui/PixelFormat.h>
> }

That would have to be ifdeffed - and I'd check (in a non-unified build!) if this actually is needed (it might be).

> class MediaEngineWebRTCVideoSource : public MediaEngineVideoSource
>                                    , public nsRunnable
>                                    ...
> {
>    ...
>    uint32_t ConvertPixexFormatToFOURCC(android::PixelFormat aFormat);
>    ...
> };

To be canonically correct: yes.  However: PixelFormat is psuedo-subclassed and extended by a series of anonymous enums in other files, so the *real* type here isn't PixelFormat.  Thus I'd stick with the generic safe enum-conversion type: int.

Comment 6

[Vincent Liu[:vliu]]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to Vincent Liu[:vliu] from comment #4)
>  
> That would have to be ifdeffed - and I'd check (in a non-unified build!) if
> this actually is needed (it might be).
> 

Since *int* is a better type to declare PixelFormat, I don't need to include this header file.

> 
> To be canonically correct: yes.  However: PixelFormat is psuedo-subclassed
> and extended by a series of anonymous enums in other files, so the *real*
> type here isn't PixelFormat.  Thus I'd stick with the generic safe
> enum-conversion type: int.

Thanks. It's more clear that using *int* is more safer than others.

Comment 7

[Vincent Liu[:vliu]]

Attachment: bug-1066410-fix-v2.patch

Patch was attached and waited for try server result.

Comment 8

[Vincent Liu[:vliu]]

Try server result on master branch:

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee9aafe6f63

Comment 10

[Vincent Liu[:vliu]]

Hi Francis,

Can you please help me to mark it as 2.0M+ since this issue blocks bug 1055462? Thanks.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ee9aafe6f63

Comment 12

[Eric Rescorla (:ekr)]

Uh, what is a "Pixex"?

Comment 13

[Vincent Liu[:vliu]]

(In reply to Eric Rescorla (:ekr) from comment #12)
> Uh, what is a "Pixex"?

Really thanks. I will reopen it and change the proper name.

Comment 14

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #13)
> (In reply to Eric Rescorla (:ekr) from comment #12)
> > Uh, what is a "Pixex"?
> 
> Really thanks. I will reopen it and change the proper name.

Discussed with other colleagues and they suggested that opening a new bug to fix typo is a better way for tracking.

Comment 15

[KTucker [:KTucker][Inactive 3/4/2016]]

Typo error was written and resolved as fixed in bug 1067718

Comment 16

[Francis Lee [:frlee]]

according to triage result, 2.0M+

Comment 18

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/492f60eea744

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on v2.1? If so, please nominate it for Aurora approval :)

Comment 20

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

Comment on attachment 8488529 [details] [diff] [review]
bug-1066410-fix-v2.patch

Approval Request Comment
[Feature/regressing bug #]: n/a

[User impact if declined]: problems when partner device updates to 2.1

[Describe test coverage new/current, TBPL]: requires manual testing on the hardware

[Risks and why]: very low risk; on 2.0M already

[String/UUID change made/needed]: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/efa3a43f6b35

Comment 22

[Vincent Liu[:vliu]]

(In reply to Randell Jesup [:jesup] from comment #20)
> Comment on attachment 8488529 [details] [diff] [review]
> bug-1066410-fix-v2.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]: n/a
> 
> [User impact if declined]: problems when partner device updates to 2.1
> 
> [Describe test coverage new/current, TBPL]: requires manual testing on the
> hardware
> 
> [Risks and why]: very low risk; on 2.0M already
> 
> [String/UUID change made/needed]: none

Thanks for the help. :)

Comment 23

[Coler]

This issue has been verified successfully on Woodduck 2.0;Flame2.1&2.2.
Reproducing rate: 0/5
See attachment: Verify_Woodduck_Color.mp4

Woodduck build version:
Gaia-Rev        87b23fa81c3b59f2ba24b84f7d686f4160d4e7cb
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141125050313
Version         32.0

Flame2.1 build:
Gaia-Rev        8ae086c39011bc8842b2a19bb5267906fa22345a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1
Build-ID        20141124094013
Version         34.0

Flame2.2:
Gaia-Rev        aad40f6d6eb8f626c6a20db55b9f00d2e832f113
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/be4ba3d5ca9a
Build-ID        20141124100136
Version         36.0a1

Comment 24

[Coler]

Attachment: Verify_Woodduck_Color.MP4