Closed Bug 1066410 Opened 9 years ago Closed 9 years ago

Serious color distortion happened when plays WebRTC

Categories

(Core :: WebRTC, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.0M+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: vliu, Assigned: vliu)

References

Details

(Whiteboard: [webrtc-uplift])

Attachments

(2 files, 1 obsolete file)

* 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.
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.
Attached patch bug-1066410-fix.patch (obsolete) — Splinter Review
Hi Randell,

Would you please help me to review the patch? Really thanks.
Attachment #8488428 - Flags: review?(rjesup)
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
Attachment #8488428 - Flags: review?(rjesup) → review+
(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);
   ...
};
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup)
(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.
Patch was attached and waited for try server result.
Attachment #8488428 - Attachment is obsolete: true
Assignee: nobody → vliu
Status: NEW → ASSIGNED
Keywords: checkin-needed
Hi Francis,

Can you please help me to mark it as 2.0M+ since this issue blocks bug 1055462? Thanks.
blocking-b2g: --- → 2.0M?
Flags: needinfo?(frlee)
https://hg.mozilla.org/mozilla-central/rev/4ee9aafe6f63
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Uh, what is a "Pixex"?
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Typo error was written and resolved as fixed in bug 1067718
according to triage result, 2.0M+
blocking-b2g: 2.0M? → 2.0M+
Flags: needinfo?(frlee)
Does this need to land on v2.1? If so, please nominate it for Aurora approval :)
status-b2g-v2.1: --- → ?
Flags: needinfo?(vliu)
Whiteboard: [webrtc-uplift]
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
Attachment #8488529 - Flags: approval-mozilla-aurora?
Attachment #8488529 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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. :)
Flags: needinfo?(vliu)
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
You need to log in before you can comment on or make changes to this bug.