Serious color distortion happened when plays WebRTC

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vliu, Assigned: vliu)

Tracking

unspecified
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0M+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 wontfix, b2g-v2.0M verified, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [webrtc-uplift])

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
* 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.
Assignee

Updated

5 years ago
Blocks: 1055462
Assignee

Comment 1

5 years ago
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.
Assignee

Comment 2

5 years ago
Posted 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+
Assignee

Comment 4

5 years ago
(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)
Assignee

Comment 6

5 years ago
(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.
Assignee

Comment 7

5 years ago
Patch was attached and waited for try server result.
Attachment #8488428 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Assignee: nobody → vliu
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 10

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Uh, what is a "Pixex"?
Assignee

Comment 13

5 years ago
(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 → ---
Assignee

Comment 14

5 years ago
(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: 5 years ago5 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)

Updated

5 years ago
Duplicate of this bug: 1055462
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+
Assignee

Comment 22

5 years ago
(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)

Comment 23

5 years ago
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

5 years ago
You need to log in before you can comment on or make changes to this bug.