Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802
Committed: https://crrev.com/acc39c44bc901caace84e2f6671e67669194ee02
Cr-Commit-Position: refs/heads/master@{#14337}

[Hzj_jie, 2016-09-12 19:41:41]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-09-12 21:54:09]

Description was changed from

==========
[WebRTC] Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issue.

BUG=
==========

to

==========
[WebRTC] Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=
==========

[Hzj_jie, 2016-09-12 21:54:48]

zijiehe@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-09-13 00:47:48]

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:52: return *(reinterpret_cast<const
uint32_t*>(&blue));
I don't think you can use reinterpret_cast<> here without making assumption
about how the struct is packed in memory.
I think you need something like (alpha<<24 | red << 16 | green << 8 | blue)

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color_unittest.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color_unittest.cc:21: 0,         1000,      
2693,       3725,       4097,      12532,
How was this table generated? are these arbitrary values or is there some logic
here?

[Sergey Ulanov, 2016-09-13 00:49:11]

Also please remove [WebRTC] prefix from CL description. It's redundant because
this CL is in WebRTC repo.

[Hzj_jie, 2016-09-13 18:14:00]

Description was changed from

==========
[WebRTC] Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=
==========

to

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=
==========

[Hzj_jie, 2016-09-13 18:48:11]

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:52: return *(reinterpret_cast<const
uint32_t*>(&blue));
On 2016/09/13 00:47:48, Sergey Ulanov wrote:
> I don't think you can use reinterpret_cast<> here without making assumption
> about how the struct is packed in memory.
> I think you need something like (alpha<<24 | red << 16 | green << 8 | blue)

I think the endian will be broken by using shift, since this uint32 should be
able to directly assign to or from DesktopFrame::data(), which is always
little-endian. Have I made a mistake here?

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color_unittest.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color_unittest.cc:21: 0,         1000,      
2693,       3725,       4097,      12532,
On 2016/09/13 00:47:48, Sergey Ulanov wrote:
> How was this table generated? are these arbitrary values or is there some
logic
> here?

Except for several specific numbers, like 0, 1000, 65535, 65536, UINT32_MAX,
others are sorted random numbers in [0, UINT32_MAX).

[Sergey Ulanov, 2016-09-19 20:35:28]

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:52: return *(reinterpret_cast<const
uint32_t*>(&blue));
On 2016/09/13 18:48:10, Hzj_jie wrote:
> On 2016/09/13 00:47:48, Sergey Ulanov wrote:
> > I don't think you can use reinterpret_cast<> here without making assumption
> > about how the struct is packed in memory.
> > I think you need something like (alpha<<24 | red << 16 | green << 8 | blue)
> 
> I think the endian will be broken by using shift, since this uint32 should be
> able to directly assign to or from DesktopFrame::data(), which is always
> little-endian. Have I made a mistake here?

All platforms we currently support are little-endian (or bi-endian used in
little-endian mode): see
https://codesearch.chromium.org/chromium/src/build/build_config.h?rcl=0&l=104
I don't know which layout we will want to use for data in DesktopFrame if a
big-endian arch support ever is added in the future (which is by itself is very
unlikely), so I don't think we need to worry about it, but you can add ifdef
ARCH_CPU_LITTLE_ENDIAN if you are concerned about it and mark the big-endian
case as not implemented?

[Hzj_jie, 2016-09-20 00:00:09]

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:52: return *(reinterpret_cast<const
uint32_t*>(&blue));
On 2016/09/19 20:35:28, Sergey Ulanov wrote:
> On 2016/09/13 18:48:10, Hzj_jie wrote:
> > On 2016/09/13 00:47:48, Sergey Ulanov wrote:
> > > I don't think you can use reinterpret_cast<> here without making
assumption
> > > about how the struct is packed in memory.
> > > I think you need something like (alpha<<24 | red << 16 | green << 8 |
blue)
> > 
> > I think the endian will be broken by using shift, since this uint32 should
be
> > able to directly assign to or from DesktopFrame::data(), which is always
> > little-endian. Have I made a mistake here?
> 
> All platforms we currently support are little-endian (or bi-endian used in
> little-endian mode): see
> https://codesearch.chromium.org/chromium/src/build/build_config.h?rcl=0&l=104
> I don't know which layout we will want to use for data in DesktopFrame if a
> big-endian arch support ever is added in the future (which is by itself is
very
> unlikely), so I don't think we need to worry about it, but you can add ifdef
> ARCH_CPU_LITTLE_ENDIAN if you are concerned about it and mark the big-endian
> case as not implemented?

Done.

[Sergey Ulanov, 2016-09-20 21:06:41]

lgtm

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right):

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_frame_generator.cc:178:
PaintRegion(frame, &updated_region_, RgbaColor(UINT32_MAX));
0xFFFFFFFF would be more appropriate here instead of UINT32_MAX.
Or RgbaColor(255, 255, 255, 255) is even better.

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:44: : RgbaColor((bgra & 0xff000000)
>> 24,
nit: the old version of this constructor with reinterpret_cast<> is perfectly
acceptable (assuming little-endianness), I don't think you had to change it. The
problem with reinterpret_cast<> in ToUInt32() was that we shouldn't make
assumption about struct layout in memory. It is not a problem here: uint32_t is
guaranteed to be represented as 4 continuous bytes.

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:60: #if
defined(WEBRTC_ARCH_LITTLE_ENDIAN)
It's better to put this inside the body of this function and then add  add
#error for big-endian case:
#else
 #error Big-endian is not implemented.

Same for the function above.

[Hzj_jie, 2016-09-20 23:50:12]

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/desktop_frame_generator.cc (right):

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/desktop_frame_generator.cc:178:
PaintRegion(frame, &updated_region_, RgbaColor(UINT32_MAX));
On 2016/09/20 21:06:41, Sergey Ulanov wrote:
> 0xFFFFFFFF would be more appropriate here instead of UINT32_MAX.
> Or RgbaColor(255, 255, 255, 255) is even better.

Done.

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/rgba_color.cc (right):

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:44: : RgbaColor((bgra & 0xff000000)
>> 24,
On 2016/09/20 21:06:41, Sergey Ulanov wrote:
> nit: the old version of this constructor with reinterpret_cast<> is perfectly
> acceptable (assuming little-endianness), I don't think you had to change it.
The
> problem with reinterpret_cast<> in ToUInt32() was that we shouldn't make
> assumption about struct layout in memory. It is not a problem here: uint32_t
is
> guaranteed to be represented as 4 continuous bytes.

Then I think we do not need to add big-endian or little-endian specific
treatment for this constructor. The input uint32_t should always in 'bgra' order
from left to right, no matter big-endian or little-endian. The consumer should
handle it correctly. What do you think?

https://codereview.chromium.org/2334853002/diff/40001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/rgba_color.cc:60: #if
defined(WEBRTC_ARCH_LITTLE_ENDIAN)
On 2016/09/20 21:06:41, Sergey Ulanov wrote:
> It's better to put this inside the body of this function and then add  add
> #error for big-endian case:
> #else
>  #error Big-endian is not implemented.
> 
> Same for the function above.

Since supporting big-endian is pretty simple, instead of adding an compiling
error, I would prefer to implement it.

[Hzj_jie, 2016-09-21 00:08:20]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-09-21 00:08:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2334853002/#ps60001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2016-09-21 00:08:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 00:28:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 00:28:53]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16913)

[Hzj_jie, 2016-09-21 03:39:54]

Description was changed from

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=
==========

to

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802
==========

[Hzj_jie, 2016-09-21 03:40:14]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-09-21 03:40:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 05:40:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 05:40:50]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Hzj_jie, 2016-09-21 19:14:05]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-09-21 19:14:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 19:23:21]

Description was changed from

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802
==========

to

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802
==========

[commit-bot: I haz the power, 2016-09-21 19:23:22]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-09-21 19:23:30]

Description was changed from

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802
==========

to

==========
Use RgbaColor in DesktopFrameGenerator and add RgbaColorTest

This change uses RgbaColor in DesktopFrameGenerator instead of raw uint32_t to
avoid potential endian issues.

BUG=633802

Committed: https://crrev.com/acc39c44bc901caace84e2f6671e67669194ee02
Cr-Commit-Position: refs/heads/master@{#14337}
==========

[commit-bot: I haz the power, 2016-09-21 19:23:31]

Patchset 3 (id:??) landed as
https://crrev.com/acc39c44bc901caace84e2f6671e67669194ee02
Cr-Commit-Position: refs/heads/master@{#14337}