Update SK_IMAGE_VERSION to test RAW

Update SK_IMAGE_VERSION to test RAW

Bump SK_IMAGE_VERSION to test the images in v2 in GoogleStorage, which
includes the images from v1 plus test images for SkRawCodec.

Only define skia_decodes_raw on platforms that support it, rather than
defining it always and checking additional conditions to determine
whether to support raw. Further, define it and SK_CODEC_DECODES_RAW
for all targets, so we can use the compile flag in other targets.

In DM, exclude the raw extensions if SK_CODEC_DECODES_RAW is not defined.

Blacklist raw extensions on NexusPlayer, which was running out of memory
when running them.

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

Committed: https://skia.googlesource.com/skia/+/1497f9f2641b539f0bc45b4f1496ceab7d56b18c

[scroggo, 2016-01-21 18:20:15]

Description was changed from

==========
Update SK_IMAGE_VERSION to test RAW

Once https://codereview.chromium.org/1520403003/ lands, we can start
testing RAW images. I have already added RAW images to a folder named
v2/; this updates the version so the script (once written) will check
the new version.

Depends on skbug.com/4558

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

to

==========
Update SK_IMAGE_VERSION to test RAW

Once https://codereview.chromium.org/1520403003/ lands, we can start
testing RAW images. I have already added RAW images to a folder named
v2/; this updates the version so the script (once written) will check
the new version.

Depends on skbug.com/4558

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

[scroggo, 2016-01-21 18:23:10]

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

[scroggo, 2016-01-21 18:23:10]

Eric, sending the review to you since it depends on your new script.

(I also just realized that I named the directories "v1" and "v2" instead of "1"
and "2". Let me know if that's a problem. We could just change this file to say
"v1" and "v2" if it's a problem, or we can change the directories themselves.)

[borenet, 2016-01-21 18:35:28]

On 2016/01/21 18:23:10, scroggo wrote:
> Eric, sending the review to you since it depends on your new script.
> 
> (I also just realized that I named the directories "v1" and "v2" instead of
"1"
> and "2". Let me know if that's a problem. We could just change this file to
say
> "v1" and "v2" if it's a problem, or we can change the directories themselves.)

Change LGTM but we don't yet have the mechanism set up in the recipe to make
this actually take effect, right?  And to clarify, you're talking about naming
the directory in GS "v2"?

[scroggo, 2016-01-21 18:58:30]

On 2016/01/21 18:35:28, borenet wrote:
> On 2016/01/21 18:23:10, scroggo wrote:
> > Eric, sending the review to you since it depends on your new script.
> > 
> > (I also just realized that I named the directories "v1" and "v2" instead of
> "1"
> > and "2". Let me know if that's a problem. We could just change this file to
> say
> > "v1" and "v2" if it's a problem, or we can change the directories
themselves.)
> 
> Change LGTM but we don't yet have the mechanism set up in the recipe to make
> this actually take effect, right? 

Right. That's why I sent this to you to review - I'm hoping you'll add that :)

> And to clarify, you're talking about naming
> the directory in GS "v2"?

I already named the directory in GS "v2" (as stated in
https://bugs.chromium.org/p/skia/issues/detail?id=4558#c8). Just in case, here's
the full paths:
- gs://chromium-skia-gm/skimage/v1/dm/ (what we're testing now)
- gs://chromium-skia-gm/skimage/v2/dm/ (what we'll test when the bots switch
over and we land this patch)

I'm suggesting that I could take out the "v", if that lets you reuse code from
the SKP script (which I think does not have a "v"). Alternatively, I could add
the "v" to this file.

[borenet, 2016-01-21 19:23:25]

On 2016/01/21 18:58:30, scroggo wrote:
> On 2016/01/21 18:35:28, borenet wrote:
> > On 2016/01/21 18:23:10, scroggo wrote:
> > > Eric, sending the review to you since it depends on your new script.
> > > 
> > > (I also just realized that I named the directories "v1" and "v2" instead
of
> > "1"
> > > and "2". Let me know if that's a problem. We could just change this file
to
> > say
> > > "v1" and "v2" if it's a problem, or we can change the directories
> themselves.)
> > 
> > Change LGTM but we don't yet have the mechanism set up in the recipe to make
> > this actually take effect, right? 
> 
> Right. That's why I sent this to you to review - I'm hoping you'll add that :)
> 
> > And to clarify, you're talking about naming
> > the directory in GS "v2"?
> 
> I already named the directory in GS "v2" (as stated in
> https://bugs.chromium.org/p/skia/issues/detail?id=4558#c8). Just in case,
here's
> the full paths:
> - gs://chromium-skia-gm/skimage/v1/dm/ (what we're testing now)
> - gs://chromium-skia-gm/skimage/v2/dm/ (what we'll test when the bots switch
> over and we land this patch)
> 
> I'm suggesting that I could take out the "v", if that lets you reuse code from
> the SKP script (which I think does not have a "v"). Alternatively, I could add
> the "v" to this file.

Looking at the recipe now.  I don't think the "v" will be an issue. 
Unfortunately, the SKP versions are named like,
"gs://chromium-skia-gm/playback_123/skps", which really clutters the GS
bucket...

[scroggo, 2016-01-26 21:25:47]

FYI to RAW team:

This CL will trigger our bots to download the RAW test images and try to decode
them. I'm running some trybots now. If they don't show any problems, I'll land
it so we'll be testing on every commit.

On 2016/01/21 19:23:25, borenet wrote:
> Looking at the recipe now.  I don't think the "v" will be an issue. 
> Unfortunately, the SKP versions are named like,
> "gs://chromium-skia-gm/playback_123/skps", which really clutters the GS
> bucket...

Haha, agreed!

[adaubert, 2016-01-27 08:22:45]

lgtm

Where is this v2 folder located? How can we maintain its content?

[msarett, 2016-01-27 14:09:16]

msarett@google.com changed reviewers:
+ msarett@google.com

[msarett, 2016-01-27 14:09:16]

The images are located at:
gsutil ls gs://chromium-skia-gm/skimage/v2/dm

I believe these instructions will help you to set-up gsutil if you've never used
it before:
https://cloud.google.com/sdk/#Quick_Start

Let me know if you run into any issues.  The idea behind using versioned folders
is that we can revert the change if the new images cause failures on the bots. 
So to add new images, you would want to create a v3 folder and a CL to update to
v3.

Once we click commit, "untriaged" RAW images will begin to show up on Gold
(https://gold.skia.org/list?query=source_type%3Dimage).  We will need to click
through the RAW images and verify that:
*** They look as intended (positive)
*** Something is wrong (negative).  File a bug or make a fix.

I'm happy to help.  I generally manage the images on Gold.  But I think makes
sense to let you take the first pass of the new RAW images in case you have an
idea of what to look for.

Any changes to how we decode RAW images will probably result in new images to
triage/verify on Gold.

[scroggo, 2016-01-27 14:09:17]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2016-01-27 14:09:20]

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

[scroggo, 2016-01-27 14:15:52]

The CQ bit was unchecked by scroggo@google.com

[scroggo, 2016-01-27 14:19:58]

On 2016/01/27 14:09:16, msarett wrote:
> The images are located at:
> gsutil ls gs://chromium-skia-gm/skimage/v2/dm
> 
> I believe these instructions will help you to set-up gsutil if you've never
used
> it before:
> https://cloud.google.com/sdk/#Quick_Start
> 
> Let me know if you run into any issues.  The idea behind using versioned
folders
> is that we can revert the change if the new images cause failures on the bots.

> So to add new images, you would want to create a v3 folder and a CL to update
to
> v3.
> 
> Once we click commit, "untriaged" RAW images will begin to show up on Gold
> (https://gold.skia.org/list?query=source_type%3Dimage).  We will need to click
> through the RAW images and verify that:
> *** They look as intended (positive)
> *** Something is wrong (negative).  File a bug or make a fix.
> 
> I'm happy to help.  I generally manage the images on Gold.  But I think makes
> sense to let you take the first pass of the new RAW images in case you have an
> idea of what to look for.
> 
> Any changes to how we decode RAW images will probably result in new images to
> triage/verify on Gold.

I would prefer that Matt and I manage the images until we have a better process
for updating them. (We have no safety checks in case someone does the wrong
thing, or two people try to update them at once.)

For some reason I did not see any results in Gold from my trybots. I started to
land and see what happens, but I realized we're not building RAW everywhere, so
we need to skip on those platforms. I think the simplest way will be to exclude
the raw extensions ifndef SK_BUILD_RAW

[scroggo, 2016-01-27 16:45:41]

On 2016/01/27 14:19:58, scroggo wrote:
> On 2016/01/27 14:09:16, msarett wrote:
> > The images are located at:
> > gsutil ls gs://chromium-skia-gm/skimage/v2/dm
> > 
> > I believe these instructions will help you to set-up gsutil if you've never
> used
> > it before:
> > https://cloud.google.com/sdk/#Quick_Start
> > 
> > Let me know if you run into any issues.  The idea behind using versioned
> folders
> > is that we can revert the change if the new images cause failures on the
bots.
> 
> > So to add new images, you would want to create a v3 folder and a CL to
update
> to
> > v3.
> > 
> > Once we click commit, "untriaged" RAW images will begin to show up on Gold
> > (https://gold.skia.org/list?query=source_type%3Dimage).  We will need to
click
> > through the RAW images and verify that:
> > *** They look as intended (positive)
> > *** Something is wrong (negative).  File a bug or make a fix.
> > 
> > I'm happy to help.  I generally manage the images on Gold.  But I think
makes
> > sense to let you take the first pass of the new RAW images in case you have
an
> > idea of what to look for.
> > 
> > Any changes to how we decode RAW images will probably result in new images
to
> > triage/verify on Gold.
> 
> I would prefer that Matt and I manage the images until we have a better
process
> for updating them. (We have no safety checks in case someone does the wrong
> thing, or two people try to update them at once.)
> 
> For some reason I did not see any results in Gold from my trybots. I started
to
> land and see what happens, but I realized we're not building RAW everywhere,
so
> we need to skip on those platforms. I think the simplest way will be to
exclude
> the raw extensions ifndef SK_BUILD_RAW

Once crrev.com/1611323004 lands (which will skip over raw extensions where
they're not supported i.e. ChromeOS), I think it will be safe to land this one.

[msarett, 2016-01-28 20:06:55]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-01-28 20:07:00]

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

[commit-bot: I haz the power, 2016-01-28 20:30:13]

Description was changed from

==========
Update SK_IMAGE_VERSION to test RAW

Once https://codereview.chromium.org/1520403003/ lands, we can start
testing RAW images. I have already added RAW images to a folder named
v2/; this updates the version so the script (once written) will check
the new version.

Depends on skbug.com/4558

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

to

==========
Update SK_IMAGE_VERSION to test RAW

Once https://codereview.chromium.org/1520403003/ lands, we can start
testing RAW images. I have already added RAW images to a folder named
v2/; this updates the version so the script (once written) will check
the new version.

Depends on skbug.com/4558

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

Committed:
https://skia.googlesource.com/skia/+/317a45e879889f9ed876549a10bbfd51aab41b45
==========

[commit-bot: I haz the power, 2016-01-28 20:30:14]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/317a45e879889f9ed876549a10bbfd51aab41b45

[msarett, 2016-01-28 20:58:25]

Images are starting to show up on Gold.  Woohoo!

https://gold.skia.org/list?query=source_type%3Dimage

[adaubert, 2016-01-29 08:15:03]

On 2016/01/28 20:58:25, msarett wrote:
> Images are starting to show up on Gold.  Woohoo!
> 
> https://gold.skia.org/list?query=source_type%3Dimage

Awesome :)

[scroggo, 2016-01-29 22:02:51]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1651603004/ by scroggo@google.com.

The reason for reverting is:
https://skia.googlesource.com/skia/+/7579786f3bd5a8fda84a1abc45b16213c3371f93
seems to have caused problems, but it allows us to test RAW. In order to reland
this, we'll need to reland that one, or have a simpler fix to disable RAW on
platforms where it's not supported (e.g. ChromeOS).

[msarett, 2016-01-29 23:49:18]

When we reland this, we ought to blacklist the RAW images on the Nexus Player. 
Since this CL landed, I think it has been running out of memory.

https://uberchromegw.corp.google.com/i/client.skia.android/builders/Test-Andr...

[scroggo, 2016-02-01 14:53:33]

Description was changed from

==========
Update SK_IMAGE_VERSION to test RAW

Once https://codereview.chromium.org/1520403003/ lands, we can start
testing RAW images. I have already added RAW images to a folder named
v2/; this updates the version so the script (once written) will check
the new version.

Depends on skbug.com/4558

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

Committed:
https://skia.googlesource.com/skia/+/317a45e879889f9ed876549a10bbfd51aab41b45
==========

to

==========
Update SK_IMAGE_VERSION to test RAW

Bump SK_IMAGE_VERSION to test the images in v2 in GoogleStorage, which
includes the images from v1 plus test images for SkRawCodec.

Only define skia_decodes_raw on platforms that support it, rather than
defining it always and checking additional conditions to determine
whether to support raw. Further, define it and SK_CODEC_DECODES_RAW
for all targets, so we can use the compile flag in other targets.

In DM, exclude the raw extensions if SK_CODEC_DECODES_RAW is not defined.

Blacklist raw extensions on NexusPlayer, which was running out of memory
when running them.

BUG=skia:4829

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

[scroggo, 2016-02-01 15:05:13]

New CL adds the following:
- pieces from crrev.com/1611323004 to
  - define SK_CODEC_DECODES_RAW everywhere
  - not define skia_codec_decodes_raw where it's not
    supported (chromeos)
  - only include raw extensions when SK_CODEC_DECODES_RAW
    is defined
- blacklist for NexusPlayer

PTAL

https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py
File tools/dm_flags.py (right):

https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py#newco...
tools/dm_flags.py:162: # NexusPlayer runs out of memory running RAW codec tests
Should we file a bug for this? Alternatively, we could try fixing this bot
another way. For example, we could not run with threads on the NexusPlayer -
that way we will only have one RAW file open at a time, which might fix the
problem (at the expense of making the bot run more slowly).

One thing that might help, by at least reducing the memory used by the encoded
data, would be to share it between various CodecSrc's (and AndroidCodecSrcs,
BRDSrcs...). The problem with this approach is that we don't want to create the
SkData initially and hang on to it, since that would mean holding on to it
longer than necessary (basically from when it's first opened til when the last
source using it unrefs it). I could imagine a set of weakly referenced SkDatas,
so a CodecSrc (etc) could first check to see if the file has already been opened
and ref it.

My first thought was that the NexusPlayer probably doesn't do anything
interestingly different, so it's okay to skip these tests, but clearly it has
less memory, which could point out a problem with decoding a single image.
(OTOH, we *know* there are problems with the amount of memory the SkRawCodec
holds, so maybe this gives us no more useful info.)

[msarett, 2016-02-01 15:43:11]

lgtm

https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py
File tools/dm_flags.py (right):

https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py#newco...
tools/dm_flags.py:162: # NexusPlayer runs out of memory running RAW codec tests
On 2016/02/01 15:05:13, scroggo wrote:
> Should we file a bug for this? Alternatively, we could try fixing this bot
> another way. For example, we could not run with threads on the NexusPlayer -
> that way we will only have one RAW file open at a time, which might fix the
> problem (at the expense of making the bot run more slowly).
> 

I like this idea, but I think it would probably be too slow.  How do we decide
how slow is "too slow"?

> One thing that might help, by at least reducing the memory used by the encoded
> data, would be to share it between various CodecSrc's (and AndroidCodecSrcs,
> BRDSrcs...). The problem with this approach is that we don't want to create
the
> SkData initially and hang on to it, since that would mean holding on to it
> longer than necessary (basically from when it's first opened til when the last
> source using it unrefs it). I could imagine a set of weakly referenced
SkDatas,
> so a CodecSrc (etc) could first check to see if the file has already been
opened
> and ref it.
> 

I like this idea the best.  When thinking about large streams, it seems
particularly bad that we read them into memory over and over again for all of
the different scales and tests etc.

How hard do you think this would be?

> My first thought was that the NexusPlayer probably doesn't do anything
> interestingly different, so it's okay to skip these tests, but clearly it has
> less memory, which could point out a problem with decoding a single image.
> (OTOH, we *know* there are problems with the amount of memory the SkRawCodec
> holds, so maybe this gives us no more useful info.)

I think it's worth filing a bug that we don't have any x86 (or x86-64) Android
bots that run the RAW code.  Not sure whether the fix is to fix the bot or add a
new bot.

Regardless, I agree that there isn't any interesting/different behavior not
covered by other Android bots or other x86 bots.

[scroggo, 2016-02-01 22:08:43]

On 2016/02/01 15:43:11, msarett wrote:
> lgtm
> 
> https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py
> File tools/dm_flags.py (right):
> 
>
https://codereview.chromium.org/1612113002/diff/20001/tools/dm_flags.py#newco...
> tools/dm_flags.py:162: # NexusPlayer runs out of memory running RAW codec
tests
> On 2016/02/01 15:05:13, scroggo wrote:
> > Should we file a bug for this? Alternatively, we could try fixing this bot
> > another way. For example, we could not run with threads on the NexusPlayer -
> > that way we will only have one RAW file open at a time, which might fix the
> > problem (at the expense of making the bot run more slowly).
> > 
> 
> I like this idea, but I think it would probably be too slow.  How do we decide
> how slow is "too slow"?

We'd have to ask the infra team. FWIW, GalaxyS runs without threads.

> 
> > One thing that might help, by at least reducing the memory used by the
encoded
> > data, would be to share it between various CodecSrc's (and AndroidCodecSrcs,
> > BRDSrcs...). The problem with this approach is that we don't want to create
> the
> > SkData initially and hang on to it, since that would mean holding on to it
> > longer than necessary (basically from when it's first opened til when the
last
> > source using it unrefs it). I could imagine a set of weakly referenced
> SkDatas,
> > so a CodecSrc (etc) could first check to see if the file has already been
> opened
> > and ref it.
> > 
> 
> I like this idea the best.  When thinking about large streams, it seems
> particularly bad that we read them into memory over and over again for all of
> the different scales and tests etc.
> 
> How hard do you think this would be?

Not sure. We don't have a natural way to weakly reference SkDatas (SkWeakRefCnt
is the baseclass of SkTypeface, and that's the only weak ref counting I know of
in Skia). I don't think it would be too terribly difficult to do something along
these lines though.

> 
> > My first thought was that the NexusPlayer probably doesn't do anything
> > interestingly different, so it's okay to skip these tests, but clearly it
has
> > less memory, which could point out a problem with decoding a single image.
> > (OTOH, we *know* there are problems with the amount of memory the SkRawCodec
> > holds, so maybe this gives us no more useful info.)
> 
> I think it's worth filing a bug that we don't have any x86 (or x86-64) Android
> bots that run the RAW code.  Not sure whether the fix is to fix the bot or add
a
> new bot.
> 
> Regardless, I agree that there isn't any interesting/different behavior not
> covered by other Android bots or other x86 bots.

Filed skbug.com/4878

[mtklein, 2016-02-01 22:30:23]

> > > One thing that might help, by at least reducing the memory used by the
> encoded
> > > data, would be to share it between various CodecSrc's (and
AndroidCodecSrcs,
> > > BRDSrcs...). The problem with this approach is that we don't want to
create
> > the
> > > SkData initially and hang on to it, since that would mean holding on to it
> > > longer than necessary (basically from when it's first opened til when the
> last
> > > source using it unrefs it). I could imagine a set of weakly referenced
> > SkDatas,
> > > so a CodecSrc (etc) could first check to see if the file has already been
> > opened
> > > and ref it.
> > > 
> > 
> > I like this idea the best.  When thinking about large streams, it seems
> > particularly bad that we read them into memory over and over again for all
of
> > the different scales and tests etc.
> > 
> > How hard do you think this would be?
> 
> Not sure. We don't have a natural way to weakly reference SkDatas
(SkWeakRefCnt
> is the baseclass of SkTypeface, and that's the only weak ref counting I know
of
> in Skia). I don't think it would be too terribly difficult to do something
along
> these lines though.

As long as it's not going to escape our tools into Skia-the-library, you can
just use std::shared_ptr / std::weak_ptr.

[msarett, 2016-02-02 02:23:03]

I've gone ahead and triaged most of the RAW images on Gold.

For the most part, it was straightforward.  Genearlly there
was one version of the output for 8888 and another version
for 565.

But, I did have two minor questions.

(1) It looks like there are very minor differences between
DNG decodes on Nexus 9 and the rest of the test platforms.
It's not noticeable to my eye, but the tool catches it.

I'm guessing that dng_sdk has some optimized code for Arm64
that behaves slightly differently than the default?  Is
this a known effect?  Or maybe a new bug?  Or maybe we just
don't care?

I've left the differing Nexus 9 outputs untriaged, so
anyone can take a look.
https://gold.skia.org/list?query=source_type%3Dimage

(2) Looks like some of the images need to be rotated.  Is
this a bug in Skia?  (In which case I probably should have
marked the images as negative.)  Or is this being handled
outside of Skia?

[scroggo, 2016-02-02 19:30:45]

The CQ bit was checked by scroggo@google.com

[scroggo, 2016-02-02 19:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from borenet@google.com,
adaubert@google.com
Link to the patchset: https://codereview.chromium.org/1612113002/#ps20001
(title: "Reland, with fixes")

[commit-bot: I haz the power, 2016-02-02 19:30:48]

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

[scroggo, 2016-02-02 19:32:12]

On 2016/02/02 02:23:03, msarett wrote:
> I've gone ahead and triaged most of the RAW images on Gold.
> 
> For the most part, it was straightforward.  Genearlly there
> was one version of the output for 8888 and another version
> for 565.
> 
> But, I did have two minor questions.
> 
> (1) It looks like there are very minor differences between
> DNG decodes on Nexus 9 and the rest of the test platforms.
> It's not noticeable to my eye, but the tool catches it.
> 
> I'm guessing that dng_sdk has some optimized code for Arm64
> that behaves slightly differently than the default?  Is
> this a known effect?  Or maybe a new bug?  Or maybe we just
> don't care?
> 
> I've left the differing Nexus 9 outputs untriaged, so
> anyone can take a look.
> https://gold.skia.org/list?query=source_type%3Dimage

RAW folks, can you take a look and/or weigh in here? Do you know how to use the
tool?

> 
> (2) Looks like some of the images need to be rotated.  Is
> this a bug in Skia?  (In which case I probably should have
> marked the images as negative.)  Or is this being handled
> outside of Skia?

Outside of Skia. See b/26762780

[scroggo, 2016-02-02 19:38:47]

On 2016/02/01 22:30:23, mtklein wrote:
> > > > One thing that might help, by at least reducing the memory used by the
> > encoded
> > > > data, would be to share it between various CodecSrc's (and
> AndroidCodecSrcs,
> > > > BRDSrcs...). The problem with this approach is that we don't want to
> create
> > > the
> > > > SkData initially and hang on to it, since that would mean holding on to
it
> > > > longer than necessary (basically from when it's first opened til when
the
> > last
> > > > source using it unrefs it). I could imagine a set of weakly referenced
> > > SkDatas,
> > > > so a CodecSrc (etc) could first check to see if the file has already
been
> > > opened
> > > > and ref it.
> > > > 
> > > 
> > > I like this idea the best.  When thinking about large streams, it seems
> > > particularly bad that we read them into memory over and over again for all
> of
> > > the different scales and tests etc.
> > > 
> > > How hard do you think this would be?
> > 
> > Not sure. We don't have a natural way to weakly reference SkDatas
> (SkWeakRefCnt
> > is the baseclass of SkTypeface, and that's the only weak ref counting I know
> of
> > in Skia). I don't think it would be too terribly difficult to do something
> along
> > these lines though.
> 
> As long as it's not going to escape our tools into Skia-the-library, you can
> just use std::shared_ptr / std::weak_ptr.

Good idea. Updated skbug.com/4878

[commit-bot: I haz the power, 2016-02-02 19:56:40]

Description was changed from

==========
Update SK_IMAGE_VERSION to test RAW

Bump SK_IMAGE_VERSION to test the images in v2 in GoogleStorage, which
includes the images from v1 plus test images for SkRawCodec.

Only define skia_decodes_raw on platforms that support it, rather than
defining it always and checking additional conditions to determine
whether to support raw. Further, define it and SK_CODEC_DECODES_RAW
for all targets, so we can use the compile flag in other targets.

In DM, exclude the raw extensions if SK_CODEC_DECODES_RAW is not defined.

Blacklist raw extensions on NexusPlayer, which was running out of memory
when running them.

BUG=skia:4829

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

to

==========
Update SK_IMAGE_VERSION to test RAW

Bump SK_IMAGE_VERSION to test the images in v2 in GoogleStorage, which
includes the images from v1 plus test images for SkRawCodec.

Only define skia_decodes_raw on platforms that support it, rather than
defining it always and checking additional conditions to determine
whether to support raw. Further, define it and SK_CODEC_DECODES_RAW
for all targets, so we can use the compile flag in other targets.

In DM, exclude the raw extensions if SK_CODEC_DECODES_RAW is not defined.

Blacklist raw extensions on NexusPlayer, which was running out of memory
when running them.

BUG=skia:4829

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

Committed:
https://skia.googlesource.com/skia/+/1497f9f2641b539f0bc45b4f1496ceab7d56b18c
==========

[commit-bot: I haz the power, 2016-02-02 19:56:41]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/1497f9f2641b539f0bc45b4f1496ceab7d56b18c

[mtklein, 2016-02-02 20:11:31]

On 2016/02/02 19:32:12, scroggo wrote:
> On 2016/02/02 02:23:03, msarett wrote:
> > I've gone ahead and triaged most of the RAW images on Gold.
> > 
> > For the most part, it was straightforward.  Genearlly there
> > was one version of the output for 8888 and another version
> > for 565.
> > 
> > But, I did have two minor questions.
> > 
> > (1) It looks like there are very minor differences between
> > DNG decodes on Nexus 9 and the rest of the test platforms.
> > It's not noticeable to my eye, but the tool catches it.
> > 
> > I'm guessing that dng_sdk has some optimized code for Arm64
> > that behaves slightly differently than the default?  Is
> > this a known effect?  Or maybe a new bug?  Or maybe we just
> > don't care?
> > 
> > I've left the differing Nexus 9 outputs untriaged, so
> > anyone can take a look.
> > https://gold.skia.org/list?query=source_type%3Dimage
> 
> RAW folks, can you take a look and/or weigh in here? Do you know how to use
the
> tool?

If floats are involved, this might be because expressions of the form a*b+c can
evaluate as a single fused-multiply-add on ARMv8.  That'll happen even if you
didn't ask for it explicitly (e.g. by calling std::fma).

Matt, did you see this on both Debug and Release Nexus 9 builds, or just
Release?  FMA is the most likely answer if you saw it only on Release builds.

[msarett, 2016-02-02 20:17:38]

On 2016/02/02 20:11:31, mtklein wrote:
> On 2016/02/02 19:32:12, scroggo wrote:
> > On 2016/02/02 02:23:03, msarett wrote:
> > > I've gone ahead and triaged most of the RAW images on Gold.
> > > 
> > > For the most part, it was straightforward.  Genearlly there
> > > was one version of the output for 8888 and another version
> > > for 565.
> > > 
> > > But, I did have two minor questions.
> > > 
> > > (1) It looks like there are very minor differences between
> > > DNG decodes on Nexus 9 and the rest of the test platforms.
> > > It's not noticeable to my eye, but the tool catches it.
> > > 
> > > I'm guessing that dng_sdk has some optimized code for Arm64
> > > that behaves slightly differently than the default?  Is
> > > this a known effect?  Or maybe a new bug?  Or maybe we just
> > > don't care?
> > > 
> > > I've left the differing Nexus 9 outputs untriaged, so
> > > anyone can take a look.
> > > https://gold.skia.org/list?query=source_type%3Dimage
> > 
> > RAW folks, can you take a look and/or weigh in here? Do you know how to use
> the
> > tool?
> 
> If floats are involved, this might be because expressions of the form a*b+c
can
> evaluate as a single fused-multiply-add on ARMv8.  That'll happen even if you
> didn't ask for it explicitly (e.g. by calling std::fma).
> 
> Matt, did you see this on both Debug and Release Nexus 9 builds, or just
> Release?  FMA is the most likely answer if you saw it only on Release builds.

That's a great insight.

I know I saw Nexus 9 show up in both alternatives (the "good" and the "slightly
different").  I was confused about why it was in both (but Debug and Release
is probably the answer).

I can't confirm however because these images have disappeared from Gold.  It's
been a few days since the revert.  We'll know soon enough since this has been
relanded.

[yujieqin, 2016-02-02 21:44:33]

On 2016/02/02 20:17:38, msarett wrote:
> On 2016/02/02 20:11:31, mtklein wrote:
> > On 2016/02/02 19:32:12, scroggo wrote:
> > > On 2016/02/02 02:23:03, msarett wrote:
> > > > I've gone ahead and triaged most of the RAW images on Gold.
> > > > 
> > > > For the most part, it was straightforward.  Genearlly there
> > > > was one version of the output for 8888 and another version
> > > > for 565.
> > > > 
> > > > But, I did have two minor questions.
> > > > 
> > > > (1) It looks like there are very minor differences between
> > > > DNG decodes on Nexus 9 and the rest of the test platforms.
> > > > It's not noticeable to my eye, but the tool catches it.
> > > > 
> > > > I'm guessing that dng_sdk has some optimized code for Arm64
> > > > that behaves slightly differently than the default?  Is
> > > > this a known effect?  Or maybe a new bug?  Or maybe we just
> > > > don't care?
> > > > 
> > > > I've left the differing Nexus 9 outputs untriaged, so
> > > > anyone can take a look.
> > > > https://gold.skia.org/list?query=source_type%3Dimage
> > > 
> > > RAW folks, can you take a look and/or weigh in here? Do you know how to
use
> > the
> > > tool?
> > 
> > If floats are involved, this might be because expressions of the form a*b+c
> can
> > evaluate as a single fused-multiply-add on ARMv8.  That'll happen even if
you
> > didn't ask for it explicitly (e.g. by calling std::fma).
> > 
> > Matt, did you see this on both Debug and Release Nexus 9 builds, or just
> > Release?  FMA is the most likely answer if you saw it only on Release
builds.
> 
> That's a great insight.
> 
> I know I saw Nexus 9 show up in both alternatives (the "good" and the
"slightly
> different").  I was confused about why it was in both (but Debug and Release
> is probably the answer).
> 
> I can't confirm however because these images have disappeared from Gold.  It's
> been a few days since the revert.  We'll know soon enough since this has been
> relanded.

Sorry for late here, somehow missed this CL from my mail. Do you still get the
issue with the golden image at Nexus 9? 

AFAIK, dng_sdk do have some places using different system call on different
platform (e.g. posix_memalign() for dng_memory.cpp). Anyhow, I am not aware of
any effect from dng_sdk that will cases the rendered result being different at
Arm64.

adaubert@, do I miss something?

[msarett, 2016-02-02 21:57:41]

I would guess that those images will show up on Gold again, though we'll need to
wait for the Nexus 9 Test bot to run again.

mtklein had an interesting thought on what might be causing the diffs.

[msarett, 2016-02-02 22:44:27]

The DNG images are back on Gold.
https://gold.skia.org/list?query=source_type%3Dimage

The diffs are occurring in Release mode on Nexus 9.

If nobody has any objections, I'll mark them as positive tomorrow.

[mtklein, 2016-02-03 01:02:12]

On 2016/02/02 22:44:27, msarett wrote:
> The DNG images are back on Gold.
> https://gold.skia.org/list?query=source_type%3Dimage
> 
> The diffs are occurring in Release mode on Nexus 9.
> 
> If nobody has any objections, I'll mark them as positive tomorrow.

Sounds good to me.

We could probably (probably) rig up a GCE bot to also use FMA when possible, so
we have a stronger indication that certain differences are caused by FMA and not
say, buggy ARMv8 assembly.  IIRC, FMA is available on Haswell+.