Support multiple PDF rendering backends in the GM

Support multiple PDF rendering backends in the GM

R=epoger@google.com, vandebo@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=10841

[ducky, 2013-08-08 03:58:00]

This adds support for multiple PDF renderers (currently macnative and poppler)
in the GM (saving each to disk and comparing to expectations where available),
and refactors the test_drawing path of the GM to pass an ErrorCombination down
to run_multiple_configs before reporting results.

Some notes and questions about style in the code comments.

This depends on the libpoppler import CL and the GM refactoring CL. The native
PDF renderer could also be enabled here, as noted in the comments.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#oldcode245
gm/gmmain.cpp:245: filename.append(shortName);
Is there a reason this only include the shortName, instead of
shortNamePlusConfig? I think this function (and several others) could be
simplified by passing in shortNamePlusConfig, but this is the only one which
uses the short name without config.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode996
gm/gmmain.cpp:996: const SkTDArray<const PDFRasterizerData*> &pdfRasterizers,
Currently, only the test_drawing chain is refactored to pass errors down,
instead of reporting them as they go. Would it be worth refactoring the
test_deferred_drawing chain to do the same, for consistency? This will probably
be a separate CL.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1273
gm/gmmain.cpp:1273: };
The new Skia native PDF renderer could be added here.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1922
gm/gmmain.cpp:1922: static bool parse_flags_pdf_rasterizers(SkTDArray<const
PDFRasterizerData*>* outRasterizers) {
This function is very similar to parse_flags_configs (a simplified version,
actually), but exists because it uses a different data structure (pdf rasterizer
data vs. config data, even though they essentially use similar entries of
either) and this outputs an array of pointers instead of an index (so that the
array of all PDF rasterizers don't need to be passed into the GMMain methods).

The first one could possibly be solved by making this a template function
(though introducing significantly more complexity), but the second one involves
refactoring which may or may not be a good idea.

Thoughts?

[edisonn, 2013-08-08 20:00:38]

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#oldcode245
gm/gmmain.cpp:245: filename.append(shortName);
On 2013/08/08 03:58:00, ducky wrote:
> Is there a reason this only include the shortName, instead of
> shortNamePlusConfig? I think this function (and several others) could be
> simplified by passing in shortNamePlusConfig, but this is the only one which
> uses the short name without config.

This is a question for Elliot. I will add him

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1033
gm/gmmain.cpp:1033: documentStream->rewind();
this can fail, add and AssertResult, if it is expected to succeed

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1034
gm/gmmain.cpp:1034:
(*pdfRasterizers[i]->fRasterizerFunction)(documentStream.get(), bitmap);
same here

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1038
gm/gmmain.cpp:1038: gm, gRec, &bitmapAndDigest));
allign

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1273
gm/gmmain.cpp:1273: };
On 2013/08/08 03:58:00, ducky wrote:
> The new Skia native PDF renderer could be added here.

thanks!

[ducky, 2013-08-08 20:55:45]

Add failure checks for stream rewind and rasterizer failure.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1033
gm/gmmain.cpp:1033: documentStream->rewind();
On 2013/08/08 20:00:39, edisonn wrote:
> this can fail, add and AssertResult, if it is expected to succeed

Done.
Since documentStream is a SkStreamAsset, which is a subclass of
SkStreamRewindable, failure isn't expected.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1034
gm/gmmain.cpp:1034:
(*pdfRasterizers[i]->fRasterizerFunction)(documentStream.get(), bitmap);
On 2013/08/08 20:00:39, edisonn wrote:
> same here

Done. Now replaced with an error message. I didn't see any relevant
ErrorCombinations to report, though.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#newcode1038
gm/gmmain.cpp:1038: gm, gRec, &bitmapAndDigest));
On 2013/08/08 20:00:39, edisonn wrote:
> allign

Done.

[vandebo (ex-Chrome), 2013-08-09 16:26:34]

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#oldcode1066
gm/gmmain.cpp:1066: SkAutoTUnref<SkStreamAsset>
documentStream(document.detachAsStream());
It looks like you can leave this block of code here.... by that I mean there is
common code in the pdf, xps, and raster case, pull it to down here.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode161
gm/gmmain.cpp:161: struct PDFRasterizerData {
PDFRasterizer PDFRasterizerInfo ?

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode163
gm/gmmain.cpp:163: const char*                     fName;
spacing

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode1058
gm/gmmain.cpp:1058: // Though we may have a rasterizer and it may output a
bitmap,
Instead of having this comment and clearing the bitmap, just use a locally
scoped (and differently named) bitmap. Line 1033

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode2117
gm/gmmain.cpp:2117: if (!parse_flags_modulo(&moduloRemainder, &moduloDivisor) ||
It looks like your flags refactor has leaked into this CL...

[ducky, 2013-08-09 23:29:41]

Style fixes again!

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#oldcode1066
gm/gmmain.cpp:1066: SkAutoTUnref<SkStreamAsset>
documentStream(document.detachAsStream());
On 2013/08/09 16:26:34, vandebo wrote:
> It looks like you can leave this block of code here.... by that I mean there
is
> common code in the pdf, xps, and raster case, pull it to down here.

Not really. The PDF case can now have multiple bitmaps, and the old code doesn't
really support that. It's also ugly in how the document is passed along with the
bitmap just so it can be written two functions (or so) down.

The newer code is a bit less DRY, but I think the repetition is minimal and
overall is more elegant than the alternatives.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode161
gm/gmmain.cpp:161: struct PDFRasterizerData {
On 2013/08/09 16:26:34, vandebo wrote:
> PDFRasterizer PDFRasterizerInfo ?

Naming was to be consistent with ConfigData, which serves the same purpose for
configs to run.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode163
gm/gmmain.cpp:163: const char*                     fName;
On 2013/08/09 16:26:34, vandebo wrote:
> spacing

Shrunk. This was to be consistent with ConfigData above.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode1058
gm/gmmain.cpp:1058: // Though we may have a rasterizer and it may output a
bitmap,
On 2013/08/09 16:26:34, vandebo wrote:
> Instead of having this comment and clearing the bitmap, just use a locally
> scoped (and differently named) bitmap. Line 1033

Done.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode2117
gm/gmmain.cpp:2117: if (!parse_flags_modulo(&moduloRemainder, &moduloDivisor) ||
On 2013/08/09 16:26:34, vandebo wrote:
> It looks like your flags refactor has leaked into this CL...

Looks like I set the wrong cl upload base...

[vandebo (ex-Chrome), 2013-08-12 17:41:56]

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#oldcode1066
gm/gmmain.cpp:1066: SkAutoTUnref<SkStreamAsset>
documentStream(document.detachAsStream());
On 2013/08/09 23:29:41, ducky wrote:
> On 2013/08/09 16:26:34, vandebo wrote:
> > It looks like you can leave this block of code here.... by that I mean there
> is
> > common code in the pdf, xps, and raster case, pull it to down here.
> 
> Not really. The PDF case can now have multiple bitmaps, and the old code
doesn't
> really support that. It's also ugly in how the document is passed along with
the

Hmm, I see.  In that sense, different PDF renders should be be different
configs.  Of course that would mean generating the PDF multiple times. Maybe the
simplified framework (you wouldn't need to pass around the list of rasterizers)
would be worth the extra computation?  Probably not worth it though.... any
thoughts?

> bitmap just so it can be written two functions (or so) down.
> 
> The newer code is a bit less DRY, but I think the repetition is minimal and
> overall is more elegant than the alternatives.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode163
gm/gmmain.cpp:163: const char*                     fName;
On 2013/08/09 23:29:41, ducky wrote:
> On 2013/08/09 16:26:34, vandebo wrote:
> > spacing
> 
> Shrunk. This was to be consistent with ConfigData above.

To be consistent, you'd line up fRasterizerFunction, since that's the name of
the field.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode164
gm/gmmain.cpp:164: bool        fRunByDefault;
spacing here too

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode295
gm/gmmain.cpp:295: bool success = SkImageEncoder::EncodeFile(path.c_str(), copy,
Since you don't use success except in the conditional, it might be cleaner to
write the code like this:

if (!EncodeFile(...)) {
  gm_fprintf(...);
  return ErrorCombination(...);
}
return kEmpty_ErrorCombination;

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode426
gm/gmmain.cpp:426: bool success = stream.writeStream(asset, asset->getLength());
And here

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1036
gm/gmmain.cpp:1036: (documentStream.get(), &pdfBitmap);
nit: ( should be on previous line.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1065
gm/gmmain.cpp:1065: }
You've dropped the compare_test_results_to_stored_expectations() from the XPS
case.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1221
gm/gmmain.cpp:1221: #if defined(SK_BUILD_FOR_MAC) || defined(SK_BUILD_POPPLER)
You should probably get rid of CAN_IMAGE_PDF, since it depends on which pdf
rasterizers are selected (maybe none).

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1331
gm/gmmain.cpp:1331: result.append("|");
"|" -> ", "

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1340
gm/gmmain.cpp:1340: result.append(" ");
" " -> ", "

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1936
gm/gmmain.cpp:1936: if (outRasterizers->count() == 0) {
Should this test that the pdf rasterizer flag wasn't specified? if I pass an
empty list of rasterizers, I'd expect none to be used.

[ducky, 2013-08-12 21:57:09]

Style fixes and bug fixes.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#oldcode1066
gm/gmmain.cpp:1066: SkAutoTUnref<SkStreamAsset>
documentStream(document.detachAsStream());
On 2013/08/12 17:41:56, vandebo wrote:
> On 2013/08/09 23:29:41, ducky wrote:
> > On 2013/08/09 16:26:34, vandebo wrote:
> > > It looks like you can leave this block of code here.... by that I mean
there
> > is
> > > common code in the pdf, xps, and raster case, pull it to down here.
> > 
> > Not really. The PDF case can now have multiple bitmaps, and the old code
> doesn't
> > really support that. It's also ugly in how the document is passed along with
> the
> 
> Hmm, I see.  In that sense, different PDF renders should be be different
> configs.  Of course that would mean generating the PDF multiple times. Maybe
the
> simplified framework (you wouldn't need to pass around the list of
rasterizers)
> would be worth the extra computation?  Probably not worth it though.... any
> thoughts?
> 
> > bitmap just so it can be written two functions (or so) down.
> > 
> > The newer code is a bit less DRY, but I think the repetition is minimal and
> > overall is more elegant than the alternatives.
> 

I don't think it would roll into configs easily, either - configs doesn't have a
way to specify functions to be called, just backend enums. You could specify
kPDFPoppler_Backend, kPDFMac_Backend, kPDFNative_Backend, but I think the code
to handle those would be messier and less elegant.

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/24001/gm/gmmain.cpp#newcode163
gm/gmmain.cpp:163: const char*                     fName;
On 2013/08/12 17:41:56, vandebo wrote:
> On 2013/08/09 23:29:41, ducky wrote:
> > On 2013/08/09 16:26:34, vandebo wrote:
> > > spacing
> > 
> > Shrunk. This was to be consistent with ConfigData above.
> 
> To be consistent, you'd line up fRasterizerFunction, since that's the name of
> the field.

Done.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode164
gm/gmmain.cpp:164: bool        fRunByDefault;
On 2013/08/12 17:41:56, vandebo wrote:
> spacing here too

Done.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode295
gm/gmmain.cpp:295: bool success = SkImageEncoder::EncodeFile(path.c_str(), copy,
On 2013/08/12 17:41:56, vandebo wrote:
> Since you don't use success except in the conditional, it might be cleaner to
> write the code like this:
> 
> if (!EncodeFile(...)) {
>   gm_fprintf(...);
>   return ErrorCombination(...);
> }
> return kEmpty_ErrorCombination;

Done. The original intent behind a separate success var was that the multiline
conditional looked a bit ugly...

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode426
gm/gmmain.cpp:426: bool success = stream.writeStream(asset, asset->getLength());
On 2013/08/12 17:41:56, vandebo wrote:
> And here

Done.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1036
gm/gmmain.cpp:1036: (documentStream.get(), &pdfBitmap);
On 2013/08/12 17:41:56, vandebo wrote:
> nit: ( should be on previous line.

Done.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1065
gm/gmmain.cpp:1065: }
On 2013/08/12 17:41:56, vandebo wrote:
> You've dropped the compare_test_results_to_stored_expectations() from the XPS
> case.

I don't think anything (useful) happens in that case -
compare_test_results_to_stored_expectations essentially just is a wrapper for
write_document when a bitmap is not passed in.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1221
gm/gmmain.cpp:1221: #if defined(SK_BUILD_FOR_MAC) || defined(SK_BUILD_POPPLER)
On 2013/08/12 17:41:56, vandebo wrote:
> You should probably get rid of CAN_IMAGE_PDF, since it depends on which pdf
> rasterizers are selected (maybe none). 

Done. 
This was left in to have an accurate ConfigFlag (kRW_ConfigFlag or
kWrite_ConfigFlag, based on whether rasterizers are built in), but looking
through the code, this config flag seems redundant for the PDF code anyways.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1331
gm/gmmain.cpp:1331: result.append("|");
On 2013/08/12 17:41:56, vandebo wrote:
> "|" -> ", "

Done.
Side note: this style makes more sense, but configUsage uses the older one. They
are no longer consistent.

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1340
gm/gmmain.cpp:1340: result.append(" ");
On 2013/08/12 17:41:56, vandebo wrote:
> " " -> ", "

I think the space makes more sense, since this outputs as:
The default value is: "poppler mac"
With spaces, it's a legitimate configuration argument. With commas, the config
parser wouldn't be very happy...

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1936
gm/gmmain.cpp:1936: if (outRasterizers->count() == 0) {
On 2013/08/12 17:41:56, vandebo wrote:
> Should this test that the pdf rasterizer flag wasn't specified? if I pass an
> empty list of rasterizers, I'd expect none to be used.

That would be ideal, but I'm not sure how to do that.
SkkommandLineFlags::StringArray doesn't seem to have a way to tell if a flag was
or was not specified. Also, the --config flag behaves in the same way - if you
just specify --config, it runs everything.

[vandebo (ex-Chrome), 2013-08-13 17:48:00]

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1065
gm/gmmain.cpp:1065: }
On 2013/08/12 21:57:10, ducky wrote:
> On 2013/08/12 17:41:56, vandebo wrote:
> > You've dropped the compare_test_results_to_stored_expectations() from the
XPS
> > case.
> 
> I don't think anything (useful) happens in that case -
> compare_test_results_to_stored_expectations essentially just is a wrapper for
> write_document when a bitmap is not passed in.

Preferably, refactor CLs should not change functionality.  If you think the XPS
case should do something else, it'd be better to pull it out to a different CL
and send it to bungeman@

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode1257
gm/gmmain.cpp:1257: { &SkPopplerRasterizePDF,   "poppler",      true },
nit: remove the extra whitespace from these declarations.

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode1317
gm/gmmain.cpp:1317: result.append(", ");
Oops, if the input should be space delimited, then this list should be as well.

[epoger, 2013-08-13 18:06:53]

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#oldcode245
gm/gmmain.cpp:245: filename.append(shortName);
On 2013/08/08 03:58:00, ducky wrote:
> Is there a reason this only include the shortName, instead of
> shortNamePlusConfig? I think this function (and several others) could be
> simplified by passing in shortNamePlusConfig, but this is the only one which
> uses the short name without config.

Yes.  The reason is that multiple configs may yield pixel-identical results, so
we store the results with a config-agnostic filename.

For example, look at
https://code.google.com/p/skia-autogen/source/browse/gm-actual/base-android-n...
:
The "arithmode" test generates pixel-identical results for configs "gpu" and
"msaa4", so they share the results uploaded as
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/a...

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode11
gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
gm/tests/run.sh
Make sure that the self-tests still pass after your changes!  If you need to
update the expectations for that test ("yes, I know the behavior changed, I
wanted it to"), run gm/tests/rebaseline.sh

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode872
gm/gmmain.cpp:872: * @param writePath unless this is NULL, write out actual
images into this
Please update the doc to reflect the removed parameters.

[ducky, 2013-08-14 00:37:11]

More fixes to pass tests and address stylistic comments.

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp
File gm/gmmain.cpp (left):

https://codereview.chromium.org/21669004/diff/19001/gm/gmmain.cpp#oldcode245
gm/gmmain.cpp:245: filename.append(shortName);
Ok, leaving as is.

On 2013/08/13 18:06:54, epoger wrote:
> On 2013/08/08 03:58:00, ducky wrote:
> > Is there a reason this only include the shortName, instead of
> > shortNamePlusConfig? I think this function (and several others) could be
> > simplified by passing in shortNamePlusConfig, but this is the only one which
> > uses the short name without config.
> 
> Yes.  The reason is that multiple configs may yield pixel-identical results,
so
> we store the results with a config-agnostic filename.
> 
> For example, look at
>
https://code.google.com/p/skia-autogen/source/browse/gm-actual/base-android-n...
> :
> The "arithmode" test generates pixel-identical results for configs "gpu" and
> "msaa4", so they share the results uploaded as
>
http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/a...

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/30001/gm/gmmain.cpp#newcode1065
gm/gmmain.cpp:1065: }
Ok, I added it back in. I realized that it does add an
IntentionallySkippedError. I thought it was a straight passthrough to
write_document, but it turns out it's not.

On 2013/08/13 17:48:00, vandebo wrote:
> On 2013/08/12 21:57:10, ducky wrote:
> > On 2013/08/12 17:41:56, vandebo wrote:
> > > You've dropped the compare_test_results_to_stored_expectations() from the
> XPS
> > > case.
> > 
> > I don't think anything (useful) happens in that case -
> > compare_test_results_to_stored_expectations essentially just is a wrapper
for
> > write_document when a bitmap is not passed in.
> 
> Preferably, refactor CLs should not change functionality.  If you think the
XPS
> case should do something else, it'd be better to pull it out to a different CL
> and send it to bungeman@

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode11
gm/gmmain.cpp:11: * If you make changes to this, re-run the self-tests at
gm/tests/run.sh
On 2013/08/13 18:06:54, epoger wrote:
> Make sure that the self-tests still pass after your changes!  If you need to
> update the expectations for that test ("yes, I know the behavior changed, I
> wanted it to"), run gm/tests/rebaseline.sh

Done.

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode872
gm/gmmain.cpp:872: * @param writePath unless this is NULL, write out actual
images into this
On 2013/08/13 18:06:54, epoger wrote:
> Please update the doc to reflect the removed parameters.

Done.

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode1257
gm/gmmain.cpp:1257: { &SkPopplerRasterizePDF,   "poppler",      true },
On 2013/08/13 17:48:00, vandebo wrote:
> nit: remove the extra whitespace from these declarations. 

Done.

https://codereview.chromium.org/21669004/diff/41001/gm/gmmain.cpp#newcode1317
gm/gmmain.cpp:1317: result.append(", ");
On 2013/08/13 17:48:00, vandebo wrote:
> Oops, if the input should be space delimited, then this list should be as
well.

Done.

https://codereview.chromium.org/21669004/diff/69001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/stderr (right):

https://codereview.chromium.org/21669004/diff/69001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/stderr:1: GM: FAILED to
write bitmap:
gm/tests/outputs/pipe-playback-failure/output-actual/mismatchPath/comparison/selftest1-pipe.png
As far as I can tell, I think this error was occurring before, just that there
was no message.

[vandebo (ex-Chrome), 2013-08-14 16:46:20]

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp#newcode1796
gm/gmmain.cpp:1796: bool prepare_subdirectories(const char *root, bool
useFileHierarchy,
Any reason to have a declaration right above the definition?  Remove.

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp#newcode1815
gm/gmmain.cpp:1815: for (int j = 0; j < pdfRasterizers.count(); j++) {
This will result in a pdf directory as well as a pdf_poppler and pdf_mac
directory.  Either poppler and mac should be in the pdf dir, or you shouldn't
make the pdf dir.  If you put this block on line 1810 and put a continue on line
1822, that will accomplish the later.

[epoger, 2013-08-14 19:45:59]

I think this will be a bit clearer once you commit
https://codereview.chromium.org/23072014/ ('Refactor GM to separate config
parsing, attempt 2') ...

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-identical-bytes-json/output-expected/stdout
(right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-identical-bytes-json/output-expected/stdout:1:
GM: These configs will be run: 8888 565
I guess this spacing will return to its prior state once you commit
https://codereview.chromium.org/23072014/ ('Refactor GM to separate config
parsing, attempt 2')

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/stderr (right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/stderr:1: GM: FAILED to
write bitmap:
gm/tests/outputs/pipe-playback-failure/output-actual/mismatchPath/comparison/selftest1-pipe.png
Do you know why this changed?  Seems surprising to me... this is the only stderr
file that changed in this CL.

[ducky, 2013-08-14 19:56:42]

Just answering the one comment.

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/pipe-pla...
File gm/tests/outputs/pipe-playback-failure/output-expected/stderr (right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/pipe-pla...
gm/tests/outputs/pipe-playback-failure/output-expected/stderr:1: GM: FAILED to
write bitmap:
gm/tests/outputs/pipe-playback-failure/output-actual/mismatchPath/comparison/selftest1-pipe.png
On 2013/08/14 19:45:59, epoger wrote:
> Do you know why this changed?  Seems surprising to me... this is the only
stderr
> file that changed in this CL.

I actually looked into this issue - it was one of the most confusing ones. My
best guess is that this failure was always happening, however, it only shows up
now that I've moved the error reporting into write_bitmap. Since the style for
the rest of the code (well, the test_drawing path, at least) seems to be check
conditions before writing the bitmap, I think this sounds reasonable. A future
refactor job might be able to address the core issue behind this.

[ducky, 2013-08-14 23:22:43]

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp#newcode1796
gm/gmmain.cpp:1796: bool prepare_subdirectories(const char *root, bool
useFileHierarchy,
On 2013/08/14 16:46:20, vandebo wrote:
> Any reason to have a declaration right above the definition?  Remove.

Not really, it just used to be that way. I think it breaks the Mac build unless
it's either declared in this nonsensical fashion, or made static. Opportunistic
refactor!

https://codereview.chromium.org/21669004/diff/90001/gm/gmmain.cpp#newcode1815
gm/gmmain.cpp:1815: for (int j = 0; j < pdfRasterizers.count(); j++) {
On 2013/08/14 16:46:20, vandebo wrote:
> This will result in a pdf directory as well as a pdf_poppler and pdf_mac
> directory.  Either poppler and mac should be in the pdf dir, or you shouldn't
> make the pdf dir.  If you put this block on line 1810 and put a continue on
line
> 1822, that will accomplish the later.

Intended behavior, required when running the GM in hierarchical mode.

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/compared...
File
gm/tests/outputs/compared-against-identical-bytes-json/output-expected/stdout
(right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/compared...
gm/tests/outputs/compared-against-identical-bytes-json/output-expected/stdout:1:
GM: These configs will be run: 8888 565
On 2013/08/14 19:45:59, epoger wrote:
> I guess this spacing will return to its prior state once you commit
> https://codereview.chromium.org/23072014/ ('Refactor GM to separate config
> parsing, attempt 2')

Done.

[vandebo (ex-Chrome), 2013-08-14 23:51:15]

LGTM

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/add-conf...
File gm/tests/outputs/add-config-pdf/output-expected/stdout (right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/add-conf...
gm/tests/outputs/add-config-pdf/output-expected/stdout:1: GM: These configs will
be run: 8888 565 pdf
Looks like you need to rebase your change.

[ducky, 2013-08-15 01:20:48]

Now just waiting for the prerequisite CLs to go in...

As a separate note, it seems that running mixed_xfermodes through the rasterizer
will take a ridiculously long amount of time. This will probably make the
buildbots very sad. What should be done about that? Is there a good way to flag
individual GMs as do-not-run for certain configs?

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/add-conf...
File gm/tests/outputs/add-config-pdf/output-expected/stdout (right):

https://codereview.chromium.org/21669004/diff/90001/gm/tests/outputs/add-conf...
gm/tests/outputs/add-config-pdf/output-expected/stdout:1: GM: These configs will
be run: 8888 565 pdf
On 2013/08/14 23:51:15, vandebo wrote:
> Looks like you need to rebase your change.

Looks fixed as of Patch Set 14 (or am I missing something?)

[vandebo (ex-Chrome), 2013-08-15 16:31:05]

On 2013/08/15 01:20:48, ducky wrote:
> Now just waiting for the prerequisite CLs to go in...
> 
> As a separate note, it seems that running mixed_xfermodes through the
rasterizer
> will take a ridiculously long amount of time. This will probably make the
> buildbots very sad. What should be done about that? Is there a good way to
flag
> individual GMs as do-not-run for certain configs?

I looked around and didn't see anything. You don't actually even want to skip
the whole "config", you just want to skip the rasterizer. Consider doing a
pre-CL that adds syntax to the expectations file to tell gm to skip a specific
config-name, i.e. pdf_poppler.

[ducky, 2013-08-15 22:02:17]

Added a flag to prevent PDF rasterization for GMs. Disable rasterization for
mixed_xfermodes, which tends to hang renderers.

[epoger, 2013-08-16 16:06:38]

LGTM at patchset 15

[ducky, 2013-08-20 22:54:33]

Ugly hack to prevent the compiler from giving "comparison of unsigned expression
< 0 is always false" warnings-as-errors when the compiled PDF rasterizer list is
empty.

[vandebo (ex-Chrome), 2013-08-20 23:01:35]

LGTM

[commit-bot: I haz the power, 2013-08-20 23:03:12]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/21669004/143006

[commit-bot: I haz the power, 2013-08-20 23:03:21]

Can't process patch for file
gm/tests/outputs/add-config-pdf/output-expected/writePath/pdf_poppler/selftest1.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[vandebo (ex-Chrome), 2013-08-20 23:08:47]

Committed patchset #17 manually as r10841 (presubmit successful).