Closed
Bug 1066410
Opened 9 years ago
Closed 9 years ago
Serious color distortion happened when plays WebRTC
Categories
(Core :: WebRTC, defect)
Tracking
()
People
(Reporter: vliu, Assigned: vliu)
References
Details
(Whiteboard: [webrtc-uplift])
Attachments
(2 files, 1 obsolete file)
3.09 KB,
patch
|
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.18 MB,
video/mp4
|
Details |
* 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 | ||
Comment 1•9 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•9 years ago
|
||
Hi Randell, Would you please help me to review the patch? Really thanks.
Attachment #8488428 -
Flags: review?(rjesup)
Comment 3•9 years ago
|
||
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•9 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)
Comment 5•9 years ago
|
||
(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•9 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•9 years ago
|
||
Patch was attached and waited for try server result.
Attachment #8488428 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Try server result on master branch: https://tbpl.mozilla.org/?tree=Try&rev=c07dbede8a20 https://tbpl.mozilla.org/?tree=Try&rev=dc642b9e04cf
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → vliu
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee9aafe6f63
Keywords: checkin-needed
Assignee | ||
Comment 10•9 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)
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ee9aafe6f63
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 12•9 years ago
|
||
Uh, what is a "Pixex"?
Assignee | ||
Comment 13•9 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•9 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: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 15•9 years ago
|
||
Typo error was written and resolved as fixed in bug 1067718
Comment 16•9 years ago
|
||
according to triage result, 2.0M+
Comment 19•9 years ago
|
||
Does this need to land on v2.1? If so, please nominate it for Aurora approval :)
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.1:
--- → ?
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → ?
status-firefox35:
--- → fixed
Flags: needinfo?(vliu)
Updated•9 years ago
|
Updated•9 years ago
|
Whiteboard: [webrtc-uplift]
Comment 20•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8488529 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•9 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•9 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•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•