Closed Bug 1066410 Opened 11 years ago Closed 11 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)
Status: ASSIGNED → RESOLVED
Closed: 11 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: 11 years ago11 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.

Attachment

General

Created:
Updated:
Size: