Various improvements to CodecSrc testing in dm

Various improvements to CodecSrc testing in dm

*** Add CodecMode and ScaledCodecMode (in place of
NormalMode), so now we test SkCodec's getPixels() and
SkScaledCodec's getPixels()

*** Don't attempt to test scanline and codec modes using
the dimensions that were recommended for SkScaledCodec.

*** Change tags so that each scale gets its own output
folder.

TODO: Make ScanlineMode and ScanlineSubsetMode support
kOutOfOrder etc.  I think this belongs with the gif CL -
I don't want to add test modes that we don't run yet.

BUG=skia:4202
BUG=skia:4238
Committed: https://skia.googlesource.com/skia/+/9e707a04127f35b199bb943b6f50ff4cf60506f8

[msarett, 2015-08-31 18:05:52]

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

[msarett, 2015-08-31 18:05:52]

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode211
dm/DMSrcSink.cpp:211: case kScanline_Subset_Mode: {
I'm wondering if maybe we should eliminate the scanline subset test after we
have implemented scanline subsetting in SkScaledCodec?  Any thoughts?

[scroggo, 2015-08-31 19:35:24]

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode209
dm/DM.cpp:209: SkString* str = new SkString(mode);
This string is leaked.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode211
dm/DM.cpp:211: return str->c_str();
This only works because you leaked str. c_str() returns a pointer to the
internal data. If you had done the following:

SkString str(mode);
str.append(scale);
return str.c_str();

the returned pointer would point to the location in memory where the internal
data used to be.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode253
dm/DM.cpp:253: push_src("image", get_output_dir("scaled_codec_kGray8",
scaleStr),
Why not pas scale directly to get_output_dir? It can then decide whether to
append based on whether it is 1.0f.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode256
dm/DM.cpp:256: push_src("image", get_output_dir("codec_kGray8", scaleStr),
This is getting messy. I think we can clean this up with a different helper
function:

push_codec_src(Path path, CodecSrc::Mode mode, CodecSrc::DstColorType dstCT,
               float scale)
{
  SkString folder;
  switch (mode) {
   case CodecSrc::kCodec_Mode:
     folder.append("codec");
     break;
   ...
  }

  switch (dstCT) {
   case CodecSrc::kGrayScale_Always_DstColorType:
     folder.append("_kGray8");
     break;
   ...
  }

  if (scale != 1.0f) {
    folder.append("_%.3f", scale);
  }

  CodecSrc* src = new CodecSrc(path, mode, dstCT, scale);
  push_src("image", folder.c_str(), src);
}

Then this will look something like:

push_codec_src(path, CodecSrc::kScaledCodec_Mode,
               CodecSrc::kGrayscale_Always_DstColorType, scale);
...

Maybe you could put all this repetitive code in a for loop, hitting all the
Modes...

Thoughts? Does that make this cleaner?

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode102
dm/DMSrcSink.cpp:102: // TODO (msarett): This should fall throught to a fatal
error once we support scaled
nit: through*

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode211
dm/DMSrcSink.cpp:211: case kScanline_Subset_Mode: {
On 2015/08/31 18:05:52, msarett wrote:
> I'm wondering if maybe we should eliminate the scanline subset test after we
> have implemented scanline subsetting in SkScaledCodec?  Any thoughts?

Yes, I think we should. This serves as a proof-of-concept test.

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode366
dm/DMSrcSink.cpp:366: SkScanlineDecoder::kTopDown_SkScanlineOrder !=
decoder->getScanlineOrder()) {
It seems like we should never reach here if kTopDown is not the order?

[msarett, 2015-08-31 21:05:29]

I think this looks a lot cleaner.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode209
dm/DM.cpp:209: SkString* str = new SkString(mode);
On 2015/08/31 19:35:24, scroggo wrote:
> This string is leaked.

Acknowledged.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode211
dm/DM.cpp:211: return str->c_str();
On 2015/08/31 19:35:24, scroggo wrote:
> This only works because you leaked str. c_str() returns a pointer to the
> internal data. If you had done the following:
> 
> SkString str(mode);
> str.append(scale);
> return str.c_str();
> 
> the returned pointer would point to the location in memory where the internal
> data used to be.

Acknowledged.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode253
dm/DM.cpp:253: push_src("image", get_output_dir("scaled_codec_kGray8",
scaleStr),
On 2015/08/31 19:35:24, scroggo wrote:
> Why not pas scale directly to get_output_dir? It can then decide whether to
> append based on whether it is 1.0f.

Acknowledged.

https://codereview.chromium.org/1327433003/diff/1/dm/DM.cpp#newcode256
dm/DM.cpp:256: push_src("image", get_output_dir("codec_kGray8", scaleStr),
On 2015/08/31 19:35:24, scroggo wrote:
> This is getting messy. I think we can clean this up with a different helper
> function:
> 
> push_codec_src(Path path, CodecSrc::Mode mode, CodecSrc::DstColorType dstCT,
>                float scale)
> {
>   SkString folder;
>   switch (mode) {
>    case CodecSrc::kCodec_Mode:
>      folder.append("codec");
>      break;
>    ...
>   }
> 
>   switch (dstCT) {
>    case CodecSrc::kGrayScale_Always_DstColorType:
>      folder.append("_kGray8");
>      break;
>    ...
>   }
> 
>   if (scale != 1.0f) {
>     folder.append("_%.3f", scale);
>   }
> 
>   CodecSrc* src = new CodecSrc(path, mode, dstCT, scale);
>   push_src("image", folder.c_str(), src);
> }
> 
> Then this will look something like:
> 
> push_codec_src(path, CodecSrc::kScaledCodec_Mode,
>                CodecSrc::kGrayscale_Always_DstColorType, scale);
> ...
> 
> Maybe you could put all this repetitive code in a for loop, hitting all the
> Modes...
> 
> Thoughts? Does that make this cleaner?

Yes this is far better!

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode102
dm/DMSrcSink.cpp:102: // TODO (msarett): This should fall throught to a fatal
error once we support scaled
On 2015/08/31 19:35:24, scroggo wrote:
> nit: through*

Done.

https://codereview.chromium.org/1327433003/diff/1/dm/DMSrcSink.cpp#newcode366
dm/DMSrcSink.cpp:366: SkScanlineDecoder::kTopDown_SkScanlineOrder !=
decoder->getScanlineOrder()) {
On 2015/08/31 19:35:24, scroggo wrote:
> It seems like we should never reach here if kTopDown is not the order?

Not unless there is a bug in start().  I agree that it's unnecessary.

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode276
dm/DM.cpp:276: CodecSrc::kSubset_Mode };
Behavior change: We will now "try" to test subset mode for kIndex8 and kGray8. 
It will simply fail non-fatally.

[scroggo, 2015-09-01 13:11:44]

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

[scroggo, 2015-09-01 13:11:44]

LGTM

Adding mtklein@ for thoughts on DM. Mike, TaggedSrc does not delete its strings,
since they are typically string literals. The codec tests build the options
string, which makes the code a lot cleaner, but that means we need to handle its
ownership.

I just realized that CodecSrcs are the only tests that use "options" - we could
make the options string an SkString. Come to think of it, we could also make the
tag an SkString (for consistency).

> Various improvements to CodecSrc testing in dm

nit: This should go in the change description as well. (Currently it is just in
the subject; I'm pretty sure that when the CQ lands your change, it will not be
included in the commit message.)

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode247
dm/DM.cpp:247: const char* folderStr = folder->c_str();
Could you add a comment here that explains why this is safe to do?

[mtklein, 2015-09-01 13:51:25]

On 2015/09/01 13:11:44, scroggo wrote:
> LGTM
> 
> Adding mtklein@ for thoughts on DM. Mike, TaggedSrc does not delete its
strings,
> since they are typically string literals. The codec tests build the options
> string, which makes the code a lot cleaner, but that means we need to handle
its
> ownership.
> 
> I just realized that CodecSrcs are the only tests that use "options" - we
could
> make the options string an SkString. Come to think of it, we could also make
the
> tag an SkString (for consistency).
> 
> > Various improvements to CodecSrc testing in dm
> 
> nit: This should go in the change description as well. (Currently it is just
in
> the subject; I'm pretty sure that when the CQ lands your change, it will not
be
> included in the commit message.)
> 
> https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
> File dm/DM.cpp (right):
> 
> https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode247
> dm/DM.cpp:247: const char* folderStr = folder->c_str();
> Could you add a comment here that explains why this is safe to do?

Everything as SkString or std::string sgtm.

[msarett, 2015-09-01 14:05:14]

"Everything as SkString or std::string sgtm."

Should that be a part of this patch set?

[msarett, 2015-09-01 14:05:46]

*Not patch set, I meant CL.

[mtklein, 2015-09-01 14:07:02]

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode249
dm/DM.cpp:249: push_src("image", folderStr, src);
Yes, folderStr will be deleted when this function returns.

[scroggo, 2015-09-01 14:10:05]

On 2015/09/01 14:05:14, msarett wrote:
> "Everything as SkString or std::string sgtm."
> 
> Should that be a part of this patch set?

I'm fine with it either way

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode249
dm/DM.cpp:249: push_src("image", folderStr, src);
On 2015/09/01 14:07:02, mtklein wrote:
> Yes, folderStr will be deleted when this function returns.

?

folderStr points into folder, which is owned by src. src will ultimately be
deleted by its TaggedSrc, at which point folderStr will be pointing nowhere
meaningful, so I think this is safe to do, but tricky.

[msarett, 2015-09-01 14:14:59]

I'm still a bit confused on this as well.

I had thought that the SkString owns the folderStr pointer, but now I'm looking
closer at SkString and not finding evidence of that?

[mtklein, 2015-09-01 14:18:11]

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/20001/dm/DM.cpp#newcode249
dm/DM.cpp:249: push_src("image", folderStr, src);
On 2015/09/01 14:10:04, scroggo wrote:
> On 2015/09/01 14:07:02, mtklein wrote:
> > Yes, folderStr will be deleted when this function returns.
> 
> ?
> 
> folderStr points into folder, which is owned by src. src will ultimately be
> deleted by its TaggedSrc, at which point folderStr will be pointing nowhere
> meaningful, so I think this is safe to do, but tricky.

Oh!  I missed the detach.  This is fine.

[msarett, 2015-09-01 14:37:59]

Let me know about your preference between PS 2 and PS 3.

Leon, I'll add the comment you suggested if we go with PS 2.

[msarett, 2015-09-01 17:00:12]

Patchset #5 (id:80001) has been deleted

[msarett, 2015-09-01 17:01:44]

Using ImplicitString makes things a little better.

[scroggo, 2015-09-01 21:41:22]

LGTM

https://codereview.chromium.org/1327433003/diff/100001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/100001/dm/DM.cpp#newcode248
dm/DM.cpp:248: push_src(SkString("image"), folder, src);
It seems like if this takes an ImplicitString, this can just be:

push_src("image", folder, src);

https://codereview.chromium.org/1327433003/diff/100001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1327433003/diff/100001/dm/DMSrcSink.h#newcode173
dm/DMSrcSink.h:173: GPUSink(GrContextFactory::GLContextType, GrGLStandard, int
samples, bool diText, bool threaded);
Why did this change? rebase?

[msarett, 2015-09-01 21:51:32]

https://codereview.chromium.org/1327433003/diff/100001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1327433003/diff/100001/dm/DM.cpp#newcode248
dm/DM.cpp:248: push_src(SkString("image"), folder, src);
On 2015/09/01 21:41:22, scroggo wrote:
> It seems like if this takes an ImplicitString, this can just be:
> 
> push_src("image", folder, src);

You are correct.  I have fixed this.

https://codereview.chromium.org/1327433003/diff/100001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/1327433003/diff/100001/dm/DMSrcSink.h#newcode173
dm/DMSrcSink.h:173: GPUSink(GrContextFactory::GLContextType, GrGLStandard, int
samples, bool diText, bool threaded);
On 2015/09/01 21:41:22, scroggo wrote:
> Why did this change? rebase?

Yes oops.

[msarett, 2015-09-01 21:51:42]

The CQ bit was checked by msarett@google.com

[msarett, 2015-09-01 21:51:42]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1327433003/#ps120001
(title: "Fixed comments")

[commit-bot: I haz the power, 2015-09-01 21:51:53]

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

[commit-bot: I haz the power, 2015-09-01 21:58:00]

Committed patchset #6 (id:120001) as
https://skia.googlesource.com/skia/+/9e707a04127f35b199bb943b6f50ff4cf60506f8