Enable RAW codec for Windows

Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889
(b/26958348)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1738913002

Committed: https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031

Committed: https://skia.googlesource.com/skia/+/f236ee4e44662e519db4b4997eee5d2bc8543f9c

[yujieqin, 2016-02-25 14:44:33]

Description was changed from

==========
Enable RAW codec for Windows

BUG=skia:
==========

to

==========
Enable RAW codec for Windows

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:00:05]

Description was changed from

==========
Enable RAW codec for Windows

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows

BUG=skia:4889 b/26958348
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:00:22]

Description was changed from

==========
Enable RAW codec for Windows

BUG=skia:4889 b/26958348
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows

BUG=skia:4889, b/26958348
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:01:00]

Description was changed from

==========
Enable RAW codec for Windows

BUG=skia:4889, b/26958348
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows
Fix the exception catching

BUG=skia:4889
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:01:56]

yujieqin@google.com changed reviewers:
+ msarett@google.com, scroggo@google.com

[yujieqin, 2016-02-26 09:03:09]

Description was changed from

==========
Enable RAW codec for Windows
Fix the exception catching

BUG=skia:4889
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:04:43]

Description was changed from

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[yujieqin, 2016-02-26 09:26:37]

The CQ bit was checked by yujieqin@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-02-26 09:26:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738913002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738913002/80001

[commit-bot: I haz the power, 2016-02-26 09:26:46]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-02-26 15:26 UTC

[scroggo, 2016-02-26 13:03:23]

scroggo@google.com changed reviewers:
+ mtklein@google.com

[scroggo, 2016-02-26 13:03:23]

looks fine to me. Adding Mike to look at use of atomics (I think he reviewed
this part of the code to begin with as well(.

[mtklein, 2016-02-26 13:29:11]

https://codereview.chromium.org/1738913002/diff/120001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1738913002/diff/120001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:110: SkAtomic<dng_error_code>
dngErrorCode(dng_error_none);
Generally I'd prefer that we use a mutex until it proves to be a performance
problem.
I can't imagine it is here, given that you'd only be locking the mutex on error.

This would also allow you to gather all the errors, or perhaps even store the
dng_exception rather than just its error code?
(I don't know if that matters.)

SkMutex mutex;
SkTArray<dng_exception> exceptions;

taskGroup.add([&mutex, &exceptions, ... ]) {
  try { ... }

  catch (dng_exception& except) {
     SkAutoMutexAcquire lock(mutex);
     exceptions.push_back(except);
  }
  ...
}

taskGroup.Wait();
task.Finish(numTasks)

if (exceptions.count()) {
   ...
}

[yujieqin, 2016-02-26 14:02:37]

https://codereview.chromium.org/1738913002/diff/120001/src/codec/SkRawCodec.cpp
File src/codec/SkRawCodec.cpp (right):

https://codereview.chromium.org/1738913002/diff/120001/src/codec/SkRawCodec.c...
src/codec/SkRawCodec.cpp:110: SkAtomic<dng_error_code>
dngErrorCode(dng_error_none);
On 2016/02/26 13:29:11, mtklein wrote:
> Generally I'd prefer that we use a mutex until it proves to be a performance
> problem.
> I can't imagine it is here, given that you'd only be locking the mutex on
error.
> 
> This would also allow you to gather all the errors, or perhaps even store the
> dng_exception rather than just its error code?
> (I don't know if that matters.)
> 
> SkMutex mutex;
> SkTArray<dng_exception> exceptions;
> 
> taskGroup.add([&mutex, &exceptions, ... ]) {
>   try { ... }
> 
>   catch (dng_exception& except) {
>      SkAutoMutexAcquire lock(mutex);
>      exceptions.push_back(except);
>   }
>   ...
> }
> 
> taskGroup.Wait();
> task.Finish(numTasks)
> 
> if (exceptions.count()) {
>    ...
> }

Thanks for the tips. 

I will store all the exceptions, but only re-throw the first catched for now.

[mtklein, 2016-02-26 14:04:14]

lgtm

[yujieqin, 2016-02-26 14:05:57]

The CQ bit was checked by yujieqin@google.com

[commit-bot: I haz the power, 2016-02-26 14:06:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738913002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738913002/140001

[commit-bot: I haz the power, 2016-02-26 14:18:22]

Description was changed from

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031
==========

[commit-bot: I haz the power, 2016-02-26 14:18:22]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031

[mtklein, 2016-02-27 00:57:20]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1747443003/ by mtklein@google.com.

The reason for reverting is: I don't think there's anything wrong with this
per-se, but the 32-bit Windows bots are running out of memory while running
these tests now.

(You'll see something like
c:\0\build\slave\workdir\build\skia\include\core\skbitmap.h:247: fatal error:
""sk_throw"" in the log.)

We run these tests in parallel, and sometimes these 32-bit processes try to use
more than the 2-3G RAM they can allocate.  Seems like this is a particularly
memory-intense process?

If we reland this, we might want to blacklist these tests on the 32-bit Windows
bots.  The 64-bit bots should have access to tons and tons of RAM and let us
keep testing for Windows..

[yujieqin, 2016-02-29 08:38:39]

On 2016/02/27 00:57:20, mtklein wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/1747443003/ by mailto:mtklein@google.com.
> 
> The reason for reverting is: I don't think there's anything wrong with this
> per-se, but the 32-bit Windows bots are running out of memory while running
> these tests now.
> 
> (You'll see something like
> c:\0\build\slave\workdir\build\skia\include\core\skbitmap.h:247: fatal error:
> ""sk_throw"" in the log.)
> 
> We run these tests in parallel, and sometimes these 32-bit processes try to
use
> more than the 2-3G RAM they can allocate.  Seems like this is a particularly
> memory-intense process?
> 
> If we reland this, we might want to blacklist these tests on the 32-bit
Windows
> bots.  The 64-bit bots should have access to tons and tons of RAM and let us
> keep testing for Windows..

Thanks for helping to revert the commit. :)

I try to disable the RAW codec test directly in the tests/CodexTest.cpp in Patch
Set 9. PTAL.

[yujieqin, 2016-02-29 08:40:28]

Description was changed from

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031
==========

to

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031
==========

[yujieqin, 2016-02-29 11:40:16]

On 2016/02/29 08:38:39, yujieqin wrote:
> On 2016/02/27 00:57:20, mtklein wrote:
> > A revert of this CL (patchset #8 id:140001) has been created in
> > https://codereview.chromium.org/1747443003/ by mailto:mtklein@google.com.
> > 
> > The reason for reverting is: I don't think there's anything wrong with this
> > per-se, but the 32-bit Windows bots are running out of memory while running
> > these tests now.
> > 
> > (You'll see something like
> > c:\0\build\slave\workdir\build\skia\include\core\skbitmap.h:247: fatal
error:
> > ""sk_throw"" in the log.)
> > 
> > We run these tests in parallel, and sometimes these 32-bit processes try to
> use
> > more than the 2-3G RAM they can allocate.  Seems like this is a particularly
> > memory-intense process?
> > 
> > If we reland this, we might want to blacklist these tests on the 32-bit
> Windows
> > bots.  The 64-bit bots should have access to tons and tons of RAM and let us
> > keep testing for Windows..
> 
> Thanks for helping to revert the commit. :)
> 
> I try to disable the RAW codec test directly in the tests/CodexTest.cpp in
Patch
> Set 9. PTAL.

It seems like the trybot is failing at the golden image test, could you please
let me know how can I disable the test for win32 there?

[scroggo, 2016-02-29 12:56:46]

On 2016/02/29 11:40:16, yujieqin wrote:
> On 2016/02/29 08:38:39, yujieqin wrote:
> > On 2016/02/27 00:57:20, mtklein wrote:
> > > A revert of this CL (patchset #8 id:140001) has been created in
> > > https://codereview.chromium.org/1747443003/ by mailto:mtklein@google.com.
> > > 
> > > The reason for reverting is: I don't think there's anything wrong with
this
> > > per-se, but the 32-bit Windows bots are running out of memory while
running
> > > these tests now.
> > > 
> > > (You'll see something like
> > > c:\0\build\slave\workdir\build\skia\include\core\skbitmap.h:247: fatal
> error:
> > > ""sk_throw"" in the log.)
> > > 
> > > We run these tests in parallel, and sometimes these 32-bit processes try
to
> > use
> > > more than the 2-3G RAM they can allocate.  Seems like this is a
particularly
> > > memory-intense process?
> > > 
> > > If we reland this, we might want to blacklist these tests on the 32-bit
> > Windows
> > > bots.  The 64-bit bots should have access to tons and tons of RAM and let
us
> > > keep testing for Windows..
> > 
> > Thanks for helping to revert the commit. :)
> > 
> > I try to disable the RAW codec test directly in the tests/CodexTest.cpp in
> Patch
> > Set 9. PTAL.
> 
> It seems like the trybot is failing at the golden image test, could you please
> let me know how can I disable the test for win32 there?

Here is one of the failures:

https://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-CPU...

In addition to CodexTest, we run tests on all the images you sent us. Those get
tested by DMSrcSink's CodecSrc. In order to stop those from running, you need to
edit tools/dm_flags.py. We already blacklist raw images for the GPU [1]. The
line that blacklists GPU says

  if 'GPU' in bot:

Where |bot| is the name of the builder. The builders that are failing look like
Test-Win8-MSVC-ShuttleB-CPU-AVX2-x86-<debug/release>, so I think you want
something like:

  if 'Win' in bot and not '64' in bot:

You'll also need to run:

> python tools/dm_flags.py test

in order to update tools/dm_flags.json.

[1] https://skia.googlesource.com/skia/+/master/tools/dm_flags.py#158

[yujieqin, 2016-02-29 14:40:56]

On 2016/02/29 12:56:46, scroggo wrote:
> On 2016/02/29 11:40:16, yujieqin wrote:
> > On 2016/02/29 08:38:39, yujieqin wrote:
> > > On 2016/02/27 00:57:20, mtklein wrote:
> > > > A revert of this CL (patchset #8 id:140001) has been created in
> > > > https://codereview.chromium.org/1747443003/ by
mailto:mtklein@google.com.
> > > > 
> > > > The reason for reverting is: I don't think there's anything wrong with
> this
> > > > per-se, but the 32-bit Windows bots are running out of memory while
> running
> > > > these tests now.
> > > > 
> > > > (You'll see something like
> > > > c:\0\build\slave\workdir\build\skia\include\core\skbitmap.h:247: fatal
> > error:
> > > > ""sk_throw"" in the log.)
> > > > 
> > > > We run these tests in parallel, and sometimes these 32-bit processes try
> to
> > > use
> > > > more than the 2-3G RAM they can allocate.  Seems like this is a
> particularly
> > > > memory-intense process?
> > > > 
> > > > If we reland this, we might want to blacklist these tests on the 32-bit
> > > Windows
> > > > bots.  The 64-bit bots should have access to tons and tons of RAM and
let
> us
> > > > keep testing for Windows..
> > > 
> > > Thanks for helping to revert the commit. :)
> > > 
> > > I try to disable the RAW codec test directly in the tests/CodexTest.cpp in
> > Patch
> > > Set 9. PTAL.
> > 
> > It seems like the trybot is failing at the golden image test, could you
please
> > let me know how can I disable the test for win32 there?
> 
> Here is one of the failures:
> 
>
https://build.chromium.org/p/client.skia/builders/Test-Win8-MSVC-ShuttleB-CPU...
> 
> In addition to CodexTest, we run tests on all the images you sent us. Those
get
> tested by DMSrcSink's CodecSrc. In order to stop those from running, you need
to
> edit tools/dm_flags.py. We already blacklist raw images for the GPU [1]. The
> line that blacklists GPU says
> 
>   if 'GPU' in bot:
> 
> Where |bot| is the name of the builder. The builders that are failing look
like
> Test-Win8-MSVC-ShuttleB-CPU-AVX2-x86-<debug/release>, so I think you want
> something like:
> 
>   if 'Win' in bot and not '64' in bot:
> 
> You'll also need to run:
> 
> > python tools/dm_flags.py test
> 
> in order to update tools/dm_flags.json.
> 
> [1] https://skia.googlesource.com/skia/+/master/tools/dm_flags.py#158

It seems like the Trybot of Win32 is fine now. PTAL.

[scroggo, 2016-02-29 14:58:44]

lgtm

[yujieqin, 2016-02-29 15:00:46]

The CQ bit was checked by yujieqin@google.com

[yujieqin, 2016-02-29 15:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1738913002/#ps200001
(title: "Black RAW codec tests for Win32 bots")

[commit-bot: I haz the power, 2016-02-29 15:00:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1738913002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1738913002/200001

[commit-bot: I haz the power, 2016-02-29 15:14:44]

Description was changed from

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031
==========

to

==========
Enable RAW codec for Windows

* Fix the exception catching
* Set preprocessor differently for MSVC

BUG=skia:4889(b/26958348)
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/474e4c3dd28b67f590851321f15d9983ef7fd031

Committed:
https://skia.googlesource.com/skia/+/f236ee4e44662e519db4b4997eee5d2bc8543f9c
==========

[commit-bot: I haz the power, 2016-02-29 15:14:46]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/f236ee4e44662e519db4b4997eee5d2bc8543f9c