Converting video color space enums to their corresponding gfx:color space eqvivalent.

Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2841813002
Cr-Commit-Position: refs/heads/master@{#470250}
Committed: https://chromium.googlesource.com/chromium/src/+/f5978920bd00bc61544631be138f042d29aecbb4

[Uzair, 2017-04-25 09:34:18]

Description was changed from

==========
Convert from strongly typed enum to int

BUG=None
==========

to

==========
Convert from strongly typed enum to int

BUG=None
==========

[Uzair, 2017-04-25 09:34:18]

uzair.jaleel@samsung.com changed reviewers:
+ a.suchit@chromium.org, hubbe@chromium.org

[Uzair, 2017-04-25 09:37:58]

On 2017/04/25 09:34:18, Uzair wrote:
> mailto:uzair.jaleel@samsung.com changed reviewers:
> + mailto:a.suchit@chromium.org, mailto:hubbe@chromium.org

Dear Reviewers,

PTAL

Thanks

[a.suchit2, 2017-04-25 14:05:08]

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_space.h
File media/base/video_color_space.h (right):

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_spac...
media/base/video_color_space.h:116: MatrixID matrix = MatrixID::INVALID;
For the small usage of enum, we would not required union.
But if we are doing it then above 3 variables are not needed.
Please remove them.

[Uzair, 2017-04-25 14:12:35]

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_space.h
File media/base/video_color_space.h (right):

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_spac...
media/base/video_color_space.h:116: MatrixID matrix = MatrixID::INVALID;
On 2017/04/25 14:05:08, a.suchit2 wrote:
> For the small usage of enum, we would not required union.
> But if we are doing it then above 3 variables are not needed.
> Please remove them.

I am not sure about any other way without using union we can convert enum class
to int.
Please can you let me know which variables are not needed, union variables or
enum class
variables ?

[a.suchit2, 2017-04-25 14:45:51]

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_space.h
File media/base/video_color_space.h (right):

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_spac...
media/base/video_color_space.h:116: MatrixID matrix = MatrixID::INVALID;
On 2017/04/25 14:12:34, Uzair wrote:
> On 2017/04/25 14:05:08, a.suchit2 wrote:
> > For the small usage of enum, we would not required union.
> > But if we are doing it then above 3 variables are not needed.
> > Please remove them.
> 
> I am not sure about any other way without using union we can convert enum
class
> to int.
> Please can you let me know which variables are not needed, union variables or
> enum class
> variables ?
Enum variables : lines 114,115,116

https://codereview.chromium.org/2841813002/diff/1/media/base/video_color_spac...
media/base/video_color_space.h:116: MatrixID matrix = MatrixID::INVALID;
On 2017/04/25 14:12:34, Uzair wrote:
> On 2017/04/25 14:05:08, a.suchit2 wrote:
> > For the small usage of enum, we would not required union.
> > But if we are doing it then above 3 variables are not needed.
> > Please remove them.
> 
> I am not sure about any other way without using union we can convert enum
class
> to int.
> Please can you let me know which variables are not needed, union variables or
> enum class
> variables ?
Enum variables : lines 114,115,116

[hubbe, 2017-04-25 17:14:23]

NOT LGTM

This CL is ugly and has no explanation.
If you want to do something this ugly, you need to put clear explanations why
you're doing it in the class and in the code.

[Uzair, 2017-04-26 10:21:39]

On 2017/04/25 17:14:23, hubbe wrote:
> NOT LGTM
> 
> This CL is ugly and has no explanation.
> If you want to do something this ugly, you need to put clear explanations why
> you're doing it in the class and in the code.

Dear Hubbe,

1) The idea was to make to the call gfx::ColorSpace::CreateVideo(int,int,int) in
   VideoColorSpace::ToGfxColorSpace() function type safe.

2) Since PrimaryID,TransferID and MatrixID are enum classes. Only way i could
think
   of avoiding the static_cast<int> to make it more type safe was to use unions.

3) As all members of unions share the same memory location i thought this could
be
   one approach for doing it.

4) Please let me know your opinion or is there any other way we could it better.

Thanks

[hubbe, 2017-04-26 18:21:31]

On 2017/04/26 10:21:39, Uzair wrote:
> On 2017/04/25 17:14:23, hubbe wrote:
> > NOT LGTM
> > 
> > This CL is ugly and has no explanation.
> > If you want to do something this ugly, you need to put clear explanations
why
> > you're doing it in the class and in the code.
> 
> Dear Hubbe,
> 
> 1) The idea was to make to the call gfx::ColorSpace::CreateVideo(int,int,int)
in
>    VideoColorSpace::ToGfxColorSpace() function type safe.

The way you did it is not type safe though.

> 
> 2) Since PrimaryID,TransferID and MatrixID are enum classes. Only way i could
> think
>    of avoiding the static_cast<int> to make it more type safe was to use
unions.
>

Using static_cast isn't really the biggest problem.
The biggest problem is that we're using integers.
 
> 3) As all members of unions share the same memory location i thought this
could
> be
>    one approach for doing it.
> 

Sure, but you're solving the wrong problem.

> 4) Please let me know your opinion or is there any other way we could it
better.
> 

The right way to do this is to use switch statements to translate
VideoColorSpace
enums directly into gfx::ColorSpace enums and then call the gfx::ColorSpace
constructor
instead of gfx::ColorSpace::CreateVideo(). Also, we should remove
gfx::ColorSpace::CreateVideo().
The code that does the enum translation should also incorporate any guessing we
need to do
to fill out unknown values.

> Thanks

[Uzair, 2017-04-28 09:04:40]

On 2017/04/26 18:21:31, hubbe wrote:
> On 2017/04/26 10:21:39, Uzair wrote:
> > On 2017/04/25 17:14:23, hubbe wrote:
> > > NOT LGTM
> > > 
> > > This CL is ugly and has no explanation.
> > > If you want to do something this ugly, you need to put clear explanations
> why
> > > you're doing it in the class and in the code.
> > 
> > Dear Hubbe,
> > 
> > 1) The idea was to make to the call
gfx::ColorSpace::CreateVideo(int,int,int)
> in
> >    VideoColorSpace::ToGfxColorSpace() function type safe.
> 
> The way you did it is not type safe though.
> 
> > 
> > 2) Since PrimaryID,TransferID and MatrixID are enum classes. Only way i
could
> > think
> >    of avoiding the static_cast<int> to make it more type safe was to use
> unions.
> >
> 
> Using static_cast isn't really the biggest problem.
> The biggest problem is that we're using integers.
>  
> > 3) As all members of unions share the same memory location i thought this
> could
> > be
> >    one approach for doing it.
> > 
> 
> Sure, but you're solving the wrong problem.
> 
> > 4) Please let me know your opinion or is there any other way we could it
> better.
> > 
> 
> The right way to do this is to use switch statements to translate
> VideoColorSpace
> enums directly into gfx::ColorSpace enums and then call the gfx::ColorSpace
> constructor
> instead of gfx::ColorSpace::CreateVideo(). Also, we should remove
> gfx::ColorSpace::CreateVideo().
> The code that does the enum translation should also incorporate any guessing
we
> need to do
> to fill out unknown values.
> 
> > Thanks



Dear Hubbe,

Thanks for the reply.

Just to make my understanding clear about the approach you suggested
In gfx::ColorSpace VideoColorSpace::ToGfxColorSpace() we should use switch as
below

  switch (video_primary) {
    default:
    case 0:  // RESERVED0
    case 1:  // BT709
    case 2:  // UNSPECIFIED
    case 3:  // RESERVED
      result.primaries_ = PrimaryID::BT709;
      break;
    case 4:  // BT470M
      result.primaries_ = PrimaryID::BT470M;
      break;
  } 
   

and completely remove gfx::ColorSpace::CreateVideo() call in
VideoColorSpace::ToGfxColorSpace()  right ?

Also in switch statement any way we need to do a static cast to the enum class
to map video color space enums
to gfx::colorspace right ?

[hubbe, 2017-04-28 17:15:36]

On 2017/04/28 09:04:40, Uzair wrote:
> On 2017/04/26 18:21:31, hubbe wrote:
> > On 2017/04/26 10:21:39, Uzair wrote:
> > > On 2017/04/25 17:14:23, hubbe wrote:
> > > > NOT LGTM
> > > > 
> > > > This CL is ugly and has no explanation.
> > > > If you want to do something this ugly, you need to put clear
explanations
> > why
> > > > you're doing it in the class and in the code.
> > > 
> > > Dear Hubbe,
> > > 
> > > 1) The idea was to make to the call
> gfx::ColorSpace::CreateVideo(int,int,int)
> > in
> > >    VideoColorSpace::ToGfxColorSpace() function type safe.
> > 
> > The way you did it is not type safe though.
> > 
> > > 
> > > 2) Since PrimaryID,TransferID and MatrixID are enum classes. Only way i
> could
> > > think
> > >    of avoiding the static_cast<int> to make it more type safe was to use
> > unions.
> > >
> > 
> > Using static_cast isn't really the biggest problem.
> > The biggest problem is that we're using integers.
> >  
> > > 3) As all members of unions share the same memory location i thought this
> > could
> > > be
> > >    one approach for doing it.
> > > 
> > 
> > Sure, but you're solving the wrong problem.
> > 
> > > 4) Please let me know your opinion or is there any other way we could it
> > better.
> > > 
> > 
> > The right way to do this is to use switch statements to translate
> > VideoColorSpace
> > enums directly into gfx::ColorSpace enums and then call the gfx::ColorSpace
> > constructor
> > instead of gfx::ColorSpace::CreateVideo(). Also, we should remove
> > gfx::ColorSpace::CreateVideo().
> > The code that does the enum translation should also incorporate any guessing
> we
> > need to do
> > to fill out unknown values.
> > 
> > > Thanks
> 
> 
> 
> Dear Hubbe,
> 
> Thanks for the reply.
> 
> Just to make my understanding clear about the approach you suggested
> In gfx::ColorSpace VideoColorSpace::ToGfxColorSpace() we should use switch as
> below
> 
>   switch (video_primary) {
>     default:
>     case 0:  // RESERVED0
>     case 1:  // BT709
>     case 2:  // UNSPECIFIED
>     case 3:  // RESERVED
>       result.primaries_ = PrimaryID::BT709;
>       break;
>     case 4:  // BT470M
>       result.primaries_ = PrimaryID::BT470M;
>       break;
>   } 
>

Essentially correct.
(We can't use result.primaires_ = ... though, we have to keep our own PrimaryID
variable.)
    
> 
> and completely remove gfx::ColorSpace::CreateVideo() call in
> VideoColorSpace::ToGfxColorSpace()  right ?
> 

Correct.

> Also in switch statement any way we need to do a static cast to the enum class
> to map video color space enums
> to gfx::colorspace right ?

We won't need static_cast anymore.

[Uzair, 2017-05-02 09:49:23]

Description was changed from

==========
Convert from strongly typed enum to int

BUG=None
==========

to

==========
Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
==========

[Uzair, 2017-05-02 09:50:04]

On 2017/04/28 17:15:36, hubbe wrote:
> On 2017/04/28 09:04:40, Uzair wrote:
> > On 2017/04/26 18:21:31, hubbe wrote:
> > > On 2017/04/26 10:21:39, Uzair wrote:
> > > > On 2017/04/25 17:14:23, hubbe wrote:
> > > > > NOT LGTM
> > > > > 
> > > > > This CL is ugly and has no explanation.
> > > > > If you want to do something this ugly, you need to put clear
> explanations
> > > why
> > > > > you're doing it in the class and in the code.
> > > > 
> > > > Dear Hubbe,
> > > > 
> > > > 1) The idea was to make to the call
> > gfx::ColorSpace::CreateVideo(int,int,int)
> > > in
> > > >    VideoColorSpace::ToGfxColorSpace() function type safe.
> > > 
> > > The way you did it is not type safe though.
> > > 
> > > > 
> > > > 2) Since PrimaryID,TransferID and MatrixID are enum classes. Only way i
> > could
> > > > think
> > > >    of avoiding the static_cast<int> to make it more type safe was to use
> > > unions.
> > > >
> > > 
> > > Using static_cast isn't really the biggest problem.
> > > The biggest problem is that we're using integers.
> > >  
> > > > 3) As all members of unions share the same memory location i thought
this
> > > could
> > > > be
> > > >    one approach for doing it.
> > > > 
> > > 
> > > Sure, but you're solving the wrong problem.
> > > 
> > > > 4) Please let me know your opinion or is there any other way we could it
> > > better.
> > > > 
> > > 
> > > The right way to do this is to use switch statements to translate
> > > VideoColorSpace
> > > enums directly into gfx::ColorSpace enums and then call the
gfx::ColorSpace
> > > constructor
> > > instead of gfx::ColorSpace::CreateVideo(). Also, we should remove
> > > gfx::ColorSpace::CreateVideo().
> > > The code that does the enum translation should also incorporate any
guessing
> > we
> > > need to do
> > > to fill out unknown values.
> > > 
> > > > Thanks
> > 
> > 
> > 
> > Dear Hubbe,
> > 
> > Thanks for the reply.
> > 
> > Just to make my understanding clear about the approach you suggested
> > In gfx::ColorSpace VideoColorSpace::ToGfxColorSpace() we should use switch
as
> > below
> > 
> >   switch (video_primary) {
> >     default:
> >     case 0:  // RESERVED0
> >     case 1:  // BT709
> >     case 2:  // UNSPECIFIED
> >     case 3:  // RESERVED
> >       result.primaries_ = PrimaryID::BT709;
> >       break;
> >     case 4:  // BT470M
> >       result.primaries_ = PrimaryID::BT470M;
> >       break;
> >   } 
> >
> 
> Essentially correct.
> (We can't use result.primaires_ = ... though, we have to keep our own
PrimaryID
> variable.)
>     
> > 
> > and completely remove gfx::ColorSpace::CreateVideo() call in
> > VideoColorSpace::ToGfxColorSpace()  right ?
> > 
> 
> Correct.
> 
> > Also in switch statement any way we need to do a static cast to the enum
class
> > to map video color space enums
> > to gfx::colorspace right ?
> 
> We won't need static_cast anymore.

Dear Hubbe,

PTAL

Thanks

[hubbe, 2017-05-02 17:15:44]

Almost there...

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
File media/base/video_color_space.cc (left):

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
media/base/video_color_space.cc:58: return
gfx::ColorSpace::CreateVideo(static_cast<int>(primaries),
We should remove the CreateVideo() function from gfx::ColorSpace() (or at least
add a TODO there for removing it.)

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
File media/base/video_color_space.cc (right):

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
media/base/video_color_space.cc:62: default:
remove default in all of these switch statements.

Without the default, the compiler will complain if someone adds additional
values without updating these switch statements, which is what we want.

[Uzair, 2017-05-03 08:31:36]

Description was changed from

==========
Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
==========

to

==========
Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Uzair, 2017-05-03 08:35:38]

uzair.jaleel@samsung.com changed reviewers:
+ danakj@chromium.org

[Uzair, 2017-05-03 08:35:39]

Dear reviewers,

PTAL

Thanks

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
File media/base/video_color_space.cc (left):

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
media/base/video_color_space.cc:58: return
gfx::ColorSpace::CreateVideo(static_cast<int>(primaries),
On 2017/05/02 17:15:44, hubbe wrote:
> We should remove the CreateVideo() function from gfx::ColorSpace() (or at
least
> add a TODO there for removing it.)

Removed CreateVideo() function from gfx::ColorSpace().

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
File media/base/video_color_space.cc (right):

https://codereview.chromium.org/2841813002/diff/20001/media/base/video_color_...
media/base/video_color_space.cc:62: default:
On 2017/05/02 17:15:44, hubbe wrote:
> remove default in all of these switch statements.
> 
> Without the default, the compiler will complain if someone adds additional
> values without updating these switch statements, which is what we want.

Done.

[danakj, 2017-05-03 14:46:27]

danakj@chromium.org changed reviewers:
+ ccameron@chromium.org

[danakj, 2017-05-03 14:46:28]

+ccameron PTAL at ui/gfx/, I can stamp.

[ccameron, 2017-05-03 14:56:24]

The ui/gfx parts lgtm, as long as media/ is okay taking on the extra logic (that
border has had its fair share of whiplash).

https://codereview.chromium.org/2841813002/diff/40001/ui/gfx/color_transform_...
File ui/gfx/color_transform_unittest.cc (right):

https://codereview.chromium.org/2841813002/diff/40001/ui/gfx/color_transform_...
ui/gfx/color_transform_unittest.cc:318: gfx::ColorSpace::MatrixID::INVALID,
gfx::ColorSpace::RangeID::LIMITED);
I think that this test will fail because INVALID will not be treated as BT709.

You can just delete this test, and add a test in media/ that unspecified values
are interpreted correctly.

[danakj, 2017-05-03 14:57:14]

Thanks, LGTM if hubbe is happy then.

[hubbe, 2017-05-03 18:17:28]

https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
media/filters/ffmpeg_video_decoder.cc:190: gfx::ColorSpace::PrimaryID
primary_id;
Don't duplucate this logic.
Create a VideoColorSpace and call ToGfxColorSpace on it.

[Uzair, 2017-05-04 11:29:37]

Dear Hubbe,

PTAL

But the unit test case in color_transform_unittest.cc is failing.

We tried below approach for fixing the color_transform_unittest.cc
1) Create video color space.
2) Then call ToGfxColorSpace.

But even with above it fails with below errors.


[ RUN      ] SimpleColorSpace.UnknownVideoToSRGB
../../ui/gfx/color_transform_unittest.cc:327: Failure
The difference between tmp.y() and 0.0f is 0.50913238525390625, which exceeds
0.001f, where
tmp.y() evaluates to 0.50913238525390625,
0.0f evaluates to 0, and
0.001f evaluates to 0.0010000000474974513.
../../ui/gfx/color_transform_unittest.cc:328: Failure
The difference between tmp.z() and 0.0f is 0.50913238525390625, which exceeds
0.001f, where
tmp.z() evaluates to 0.50913238525390625,
0.0f evaluates to 0, and
0.001f evaluates to 0.0010000000474974513.
../../ui/gfx/color_transform_unittest.cc:333: Failure
The difference between tmp.y() and 1.0f is 0.49086761474609375, which exceeds
0.001f, where
tmp.y() evaluates to 0.50913238525390625,
1.0f evaluates to 1, and
0.001f evaluates to 0.0010000000474974513.
../../ui/gfx/color_transform_unittest.cc:334: Failure
The difference between tmp.z() and 1.0f is 0.49086761474609375, which exceeds
0.001f, where
tmp.z() evaluates to 0.50913238525390625,
1.0f evaluates to 1, and
0.001f evaluates to 0.0010000000474974513.
../../ui/gfx/color_transform_unittest.cc:339: Failure
Expected: (tmp.z()) > (tmp.x()), actual: 0.509132 vs 0.511416
../../ui/gfx/color_transform_unittest.cc:340: Failure
Expected: (tmp.z()) > (tmp.y()), actual: 0.509132 vs 1.02283
[  FAILED  ] SimpleColorSpace.UnknownVideoToSRGB (2 ms)
[4169/4193] SimpleColorSpace.UnknownVideoToSRGB (2 ms)

https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
media/filters/ffmpeg_video_decoder.cc:190: gfx::ColorSpace::PrimaryID
primary_id;
On 2017/05/03 18:17:27, hubbe wrote:
> Don't duplucate this logic.
> Create a VideoColorSpace and call ToGfxColorSpace on it.

Done.

[hubbe, 2017-05-04 17:19:24]

On 2017/05/04 11:29:37, Uzair wrote:
> Dear Hubbe,
> 
> PTAL
> 
> But the unit test case in color_transform_unittest.cc is failing.
> 
> We tried below approach for fixing the color_transform_unittest.cc
> 1) Create video color space.
> 2) Then call ToGfxColorSpace.
> 
> But even with above it fails with below errors.
> 
> 
> [ RUN      ] SimpleColorSpace.UnknownVideoToSRGB
> ../../ui/gfx/color_transform_unittest.cc:327: Failure
> The difference between tmp.y() and 0.0f is 0.50913238525390625, which exceeds
> 0.001f, where
> tmp.y() evaluates to 0.50913238525390625,
> 0.0f evaluates to 0, and
> 0.001f evaluates to 0.0010000000474974513.
> ../../ui/gfx/color_transform_unittest.cc:328: Failure
> The difference between tmp.z() and 0.0f is 0.50913238525390625, which exceeds
> 0.001f, where
> tmp.z() evaluates to 0.50913238525390625,
> 0.0f evaluates to 0, and
> 0.001f evaluates to 0.0010000000474974513.
> ../../ui/gfx/color_transform_unittest.cc:333: Failure
> The difference between tmp.y() and 1.0f is 0.49086761474609375, which exceeds
> 0.001f, where
> tmp.y() evaluates to 0.50913238525390625,
> 1.0f evaluates to 1, and
> 0.001f evaluates to 0.0010000000474974513.
> ../../ui/gfx/color_transform_unittest.cc:334: Failure
> The difference between tmp.z() and 1.0f is 0.49086761474609375, which exceeds
> 0.001f, where
> tmp.z() evaluates to 0.50913238525390625,
> 1.0f evaluates to 1, and
> 0.001f evaluates to 0.0010000000474974513.
> ../../ui/gfx/color_transform_unittest.cc:339: Failure
> Expected: (tmp.z()) > (tmp.x()), actual: 0.509132 vs 0.511416
> ../../ui/gfx/color_transform_unittest.cc:340: Failure
> Expected: (tmp.z()) > (tmp.y()), actual: 0.509132 vs 1.02283
> [  FAILED  ] SimpleColorSpace.UnknownVideoToSRGB (2 ms)
> [4169/4193] SimpleColorSpace.UnknownVideoToSRGB (2 ms)
> 
>
https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
> File media/filters/ffmpeg_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
> media/filters/ffmpeg_video_decoder.cc:190: gfx::ColorSpace::PrimaryID
> primary_id;
> On 2017/05/03 18:17:27, hubbe wrote:
> > Don't duplucate this logic.
> > Create a VideoColorSpace and call ToGfxColorSpace on it.
> 
> Done.

Everything looks fine, except for the test.
You need to fix or move this test, as suggested by ccameron@ before you can
check the code in.

[Uzair, 2017-05-08 12:42:58]

On 2017/05/04 17:19:24, hubbe wrote:
> On 2017/05/04 11:29:37, Uzair wrote:
> > Dear Hubbe,
> > 
> > PTAL
> > 
> > But the unit test case in color_transform_unittest.cc is failing.
> > 
> > We tried below approach for fixing the color_transform_unittest.cc
> > 1) Create video color space.
> > 2) Then call ToGfxColorSpace.
> > 
> > But even with above it fails with below errors.
> > 
> > 
> > [ RUN      ] SimpleColorSpace.UnknownVideoToSRGB
> > ../../ui/gfx/color_transform_unittest.cc:327: Failure
> > The difference between tmp.y() and 0.0f is 0.50913238525390625, which
exceeds
> > 0.001f, where
> > tmp.y() evaluates to 0.50913238525390625,
> > 0.0f evaluates to 0, and
> > 0.001f evaluates to 0.0010000000474974513.
> > ../../ui/gfx/color_transform_unittest.cc:328: Failure
> > The difference between tmp.z() and 0.0f is 0.50913238525390625, which
exceeds
> > 0.001f, where
> > tmp.z() evaluates to 0.50913238525390625,
> > 0.0f evaluates to 0, and
> > 0.001f evaluates to 0.0010000000474974513.
> > ../../ui/gfx/color_transform_unittest.cc:333: Failure
> > The difference between tmp.y() and 1.0f is 0.49086761474609375, which
exceeds
> > 0.001f, where
> > tmp.y() evaluates to 0.50913238525390625,
> > 1.0f evaluates to 1, and
> > 0.001f evaluates to 0.0010000000474974513.
> > ../../ui/gfx/color_transform_unittest.cc:334: Failure
> > The difference between tmp.z() and 1.0f is 0.49086761474609375, which
exceeds
> > 0.001f, where
> > tmp.z() evaluates to 0.50913238525390625,
> > 1.0f evaluates to 1, and
> > 0.001f evaluates to 0.0010000000474974513.
> > ../../ui/gfx/color_transform_unittest.cc:339: Failure
> > Expected: (tmp.z()) > (tmp.x()), actual: 0.509132 vs 0.511416
> > ../../ui/gfx/color_transform_unittest.cc:340: Failure
> > Expected: (tmp.z()) > (tmp.y()), actual: 0.509132 vs 1.02283
> > [  FAILED  ] SimpleColorSpace.UnknownVideoToSRGB (2 ms)
> > [4169/4193] SimpleColorSpace.UnknownVideoToSRGB (2 ms)
> > 
> >
>
https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
> > File media/filters/ffmpeg_video_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2841813002/diff/40001/media/filters/ffmpeg_vi...
> > media/filters/ffmpeg_video_decoder.cc:190: gfx::ColorSpace::PrimaryID
> > primary_id;
> > On 2017/05/03 18:17:27, hubbe wrote:
> > > Don't duplucate this logic.
> > > Create a VideoColorSpace and call ToGfxColorSpace on it.
> > 
> > Done.
> 
> Everything looks fine, except for the test.
> You need to fix or move this test, as suggested by ccameron@ before you can
> check the code in.

Done

Dear Hubbe,
Need review for media/base/video_color_space_unittest.cc

Dear Cameron,
I have moved the invalid video color space test from ui/gfx to media/

thanks

[hubbe, 2017-05-08 17:22:40]

lgtm

[Uzair, 2017-05-08 20:15:33]

The CQ bit was checked by uzair.jaleel@samsung.com

[Uzair, 2017-05-08 20:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2841813002/#ps80001
(title: "Moved Invalid video color space test to media/")

[commit-bot: I haz the power, 2017-05-08 20:16:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 20:59:06]

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

[commit-bot: I haz the power, 2017-05-08 20:59:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Uzair, 2017-05-09 04:24:14]

The CQ bit was checked by uzair.jaleel@samsung.com

[Uzair, 2017-05-09 04:24:15]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
hubbe@chromium.org, ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/2841813002/#ps100001
(title: "Fixed ==> "warning C4701: potentially uninitialized local variable
'primary_id' used"")

[commit-bot: I haz the power, 2017-05-09 04:25:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 06:40:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494303854883940,
"parent_rev": "546a3b42a3414c671362459b1519eaeefb43afc3", "commit_rev":
"f5978920bd00bc61544631be138f042d29aecbb4"}

[commit-bot: I haz the power, 2017-05-09 06:40:20]

Description was changed from

==========
Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Below change safely converts video color space enums directly to gfx color
space enums and then calls gfx color constructor with updated values
of primaries,transfer and matrix id.


BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2841813002
Cr-Commit-Position: refs/heads/master@{#470250}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5978920bd00bc61544631be138f...
==========

[commit-bot: I haz the power, 2017-05-09 06:40:22]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f5978920bd00bc61544631be138f...