Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18585002:
Implemented transparent gradients (Closed)
|
Created:
7 years, 5 months ago by ducky Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com, edisonn Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionImplemented transparent gradients
Committed: http://code.google.com/p/skia/source/detail?r=10297
Patch Set 1 #
Total comments: 58
Patch Set 2 : Addresses stylistic issues #Patch Set 3 : Removed some leftover debugging stuff #Patch Set 4 : Fixes isValid() in SkPDFGradientShader, fixes tinybitmap regression #
Total comments: 59
Patch Set 5 : Addresses more stylistic issues. #
Total comments: 25
Patch Set 6 : Addresses stylistic issues. Moves SkPDFResourceDict into separate CL. #Patch Set 7 : Minor clean-up of previous patch set #
Total comments: 7
Patch Set 8 : More code review fixes. #
Total comments: 12
Patch Set 9 : Code review fixes and rebase for SkPDFResourceDict. #Patch Set 10 : Clean-up for previous CL #
Total comments: 2
Patch Set 11 : Cleaup for passing graphics state to create_pattern_fill_content #Patch Set 12 : Deduplicate resource name generation in create_pattern_fill_content #Patch Set 13 : Deduplicate pattern content generation code #Patch Set 14 : More deduplication #
Total comments: 2
Patch Set 15 : Variable naming change #
Total comments: 24
Patch Set 16 : Style fixes and more comments #Patch Set 17 : Revert SkPDFStream to SkData conversions, add SK_AlphaOPAQUE #Patch Set 18 : Update detachAsStream() #Patch Set 19 : Merge new SkPDFResourceDict #Patch Set 20 : Merge new SkPDFResourceDict method names #
Messages
Total messages: 26 (0 generated)
Parts of this code look pretty terrible. In particular: - A new FormXObject constructor which takes in a PDF content stream and resources. Except it needs both a resources dict and array - is there a way to extract the useful resources from just the dict? -- Context: this is to draw the greyscale gradient. The other approach was to use a PDFDevice, but that would have required more overhead, with the PDF Gradient to Paint conversion. - It takes me 4 lines of code to turn a SkWStream to a SkStream. Is there a better way to write a SkStream? Premultiply-before-interpolate mode for gradients not implemented yet. Pending further testing...
+cc edisonn - Please at least take a look so that you can stay familiar with Richard's look. On 2013/07/02 23:20:21, ducky wrote: > Parts of this code look pretty terrible. In particular: > - A new FormXObject constructor which takes in a PDF content stream and > resources. Except it needs both a resources dict and array - is there a way to > extract the useful resources from just the dict? Not directly - It's not worth trying to extract the objects from the resource dict structure... What you could do though, is make a new class that inherits from SkPDFDict that implements the resource list interface add add the objects are resources of the resource dict, then in the FormXObject constructor extract them. It's effectively hiding the resource list in the object that you do pass, but it's not too far from how the rest of the code already works. > -- Context: this is to draw the greyscale gradient. The other approach was to > use a PDFDevice, but that would have required more overhead, with the PDF > Gradient to Paint conversion. I think using the utility methods I pointed out will make it nicer and probably not too bad. > - It takes me 4 lines of code to turn a SkWStream to a SkStream. Is there a > better way to write a SkStream? Yes, I commented on that. SkPDFStream needs to be updated and improved. > Premultiply-before-interpolate mode for gradients not implemented yet. Pending > further testing... Feel free to do this in a follow up CL. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:19: static SkPDFArray* rectToArray(SkRect rect) { Put this in SkPDFUtil and use it in SkPDFDevice::copyMediaBox(). https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:47: if (!device->initialTransform().isIdentity()) { In your uses of the new constructor, do you take into consideration the initialtransform? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:72: setData(content); This whole body can be shared with the other constructor. Pull it out into a private init method (since you can't call one constructor from another). https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:73: resourceDict->ref(); This should be next to where the reference is used. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h#ne... src/pdf/SkPDFFormXObject.h:40: explicit SkPDFFormXObject(SkStream* content, SkPDFDict* resourceDict, nit: I like parameters to have a logical order: content, bbox, resourceDict, resources. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h#ne... src/pdf/SkPDFFormXObject.h:40: explicit SkPDFFormXObject(SkStream* content, SkPDFDict* resourceDict, This constructor needs a comment block. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.cpp... src/pdf/SkPDFGraphicState.cpp:210: // do a SMask if paint involves a gradient alpha How would paint involve a gradient alpha? We have a todo style: // TODO(uername): Thing to do in full sentence form, referring to a bug if possible. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.h File src/pdf/SkPDFGraphicState.h (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.h#n... src/pdf/SkPDFGraphicState.h:56: * @param alpha True to use alpha for the sMask, otherwise luminosity. Contrary to what you might see here, we prefer enums to bools for parameter arguments. It makes things easier to read at the call site. If you'd like to clean up the other bool parameter in another CL, that'd be nice. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (left): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#oldco... src/pdf/SkPDFShader.cpp:665: SkMatrix finalMatrix = fState.get()->fCanvasTransform; Do you not need to do this stuff for the gradient shader? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:404: SkAutoFree fColorData; // this provides storage for arrays in fInfo Full sentences please. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:415: State(const State& other); Make this private. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:417: SkPDFShader::State* CreateAlphaToRGBState() const; nit: CreateLuminosityState()? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:417: SkPDFShader::State* CreateAlphaToRGBState() const; nit: Put these methods after the operator== method. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:520: SkPDFObject* SkPDFShader::GetPDFShaderByState(State* inState) { Definition order should match declaration order, so you probably need to move this method around. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:628: Should this also include a ProcSet entry? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:634: content->writeText("/Pattern CS /Pattern cs /P0 SCN /P0 scn "); nit: omit extra whitespace. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:636: content->writeScalarAsText(bounds.left()); SkPDFUtils::AppendRectangle() SkPDFUtils::PaintPath() https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:655: SkAutoTUnref<SkPDFObject> nit indent: SkAutoTUnref<SkPDFObject> alphaShader(SkPDFShader::GetPDFShaderByState( state->CreateAlphaToRGBState())); https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:665: SkAutoTUnref<SkData> alphaData(alphaContent->copyToData()); This mess is caused by incomplete conversion of SkStream to SkData from SkStream. Can you do a pre-CL that finishes that conversion? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:671: fAlphaGs.reset(SkPDFGraphicState::GetSMaskGraphicState(alphaMask.get(), false, false)); Not sure, but I think you want to pull this all out to a helper function/method that take state as an argument and return the smask graphic state as a result. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:676: SkAutoTUnref<SkPDFArray> bboxArray(new SkPDFArray); Use rectToArray here as well. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:685: writePatternFillContent(colorContent.get(), bbox); Make this method return an SkDyrnamicMemoryWStream and take an option to emit "/G0 gs" or not. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:691: insertName("Type", "Pattern"); Most of this can get pulled out and shared with SkPDFImageShader::SkPDFImageShader() https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:812: SkPDFFunctionShader* SkPDFFunctionShader::CreateAlphaToRGBShader() { This isn't used, remove. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1125: fBBox(other.fBBox) Also need to copy over fImageTileModes https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1130: // allocate new storage to prevent pointer aliasing nit: comment unnecessary. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1131: fColorData.set(sk_malloc_throw( All this allocation is the same as the other constructor, but it into a shared method. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1143: SkASSERT(fType != SkShader::kNone_GradientType); Put this after line 1126
Addresses review comments: - Refactoring and stylistic changes - Added SkPDFResourceDict to cleanly manage resource dicts and references. It may be worth refactoring other code to use this in the future? - Moved some common functionality into SkPDFUtils, including RectToArray and generating ProcSet Depends on CL 18328026 for SkPDFStream::setData(SkData*). https://codereview.chromium.org/18328026/ https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:19: static SkPDFArray* rectToArray(SkRect rect) { On 2013/07/03 17:07:01, vandebo wrote: > Put this in SkPDFUtil and use it in SkPDFDevice::copyMediaBox(). Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:47: if (!device->initialTransform().isIdentity()) { On 2013/07/03 17:07:01, vandebo wrote: > In your uses of the new constructor, do you take into consideration the > initialtransform? I don't think there's a way to specify the initial transform from just a content stream. It just defaults to identity, right? Unless you think it would be a good idea to include an initial transform matrix in the constructor? https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:72: setData(content); On 2013/07/03 17:07:01, vandebo wrote: > This whole body can be shared with the other constructor. Pull it out into a > private init method (since you can't call one constructor from another). Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:73: resourceDict->ref(); On 2013/07/03 17:07:01, vandebo wrote: > This should be next to where the reference is used. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h#ne... src/pdf/SkPDFFormXObject.h:40: explicit SkPDFFormXObject(SkStream* content, SkPDFDict* resourceDict, On 2013/07/03 17:07:01, vandebo wrote: > nit: I like parameters to have a logical order: content, bbox, resourceDict, > resources. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.h#ne... src/pdf/SkPDFFormXObject.h:40: explicit SkPDFFormXObject(SkStream* content, SkPDFDict* resourceDict, On 2013/07/03 17:07:01, vandebo wrote: > nit: I like parameters to have a logical order: content, bbox, resourceDict, > resources. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.cpp File src/pdf/SkPDFGraphicState.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.cpp... src/pdf/SkPDFGraphicState.cpp:210: // do a SMask if paint involves a gradient alpha On 2013/07/03 17:07:01, vandebo wrote: > How would paint involve a gradient alpha? We have a todo style: > // TODO(uername): Thing to do in full sentence form, referring to a bug if > possible. That wasn't supposed to be there. I somehow missed that going through the presubmit git diff... https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.h File src/pdf/SkPDFGraphicState.h (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFGraphicState.h#n... src/pdf/SkPDFGraphicState.h:56: * @param alpha True to use alpha for the sMask, otherwise luminosity. On 2013/07/03 17:07:01, vandebo wrote: > Contrary to what you might see here, we prefer enums to bools for parameter > arguments. It makes things easier to read at the call site. If you'd like to > clean up the other bool parameter in another CL, that'd be nice. Done. Does it make sense to turn the other bool parameter into a enum? It makes intuitive sense as a bool, and calling code uses an inline boolean equality test. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (left): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#oldco... src/pdf/SkPDFShader.cpp:665: SkMatrix finalMatrix = fState.get()->fCanvasTransform; On 2013/07/03 17:07:01, vandebo wrote: > Do you not need to do this stuff for the gradient shader? Matrix transforms are handled in the color and alpha patterns. From an overall code point of view, it means that the State copy constructor can be cleaner, and I don't need to (possibly unintuitively) clear the transform matrices when creating an luminosity / opaque state. I'm also assuming that in the rendere, some kind of matrix is applied for every draw operation, so having the identity in the alpha and color patterns probably won't be less efficient than having it here. Not that it's significant at all.. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:404: SkAutoFree fColorData; // this provides storage for arrays in fInfo On 2013/07/03 17:07:01, vandebo wrote: > Full sentences please. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:415: State(const State& other); On 2013/07/03 17:07:01, vandebo wrote: > Make this private. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:417: SkPDFShader::State* CreateAlphaToRGBState() const; On 2013/07/03 17:07:01, vandebo wrote: > nit: CreateLuminosityState()? Changed to CreateAlphaToLuminosityState(). I think luminosity by itself can be confusing, but RGB is too clear either. Though it does become somewhat wordy and long... https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:417: SkPDFShader::State* CreateAlphaToRGBState() const; On 2013/07/03 17:07:01, vandebo wrote: > nit: Put these methods after the operator== method. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:520: SkPDFObject* SkPDFShader::GetPDFShaderByState(State* inState) { On 2013/07/03 17:07:01, vandebo wrote: > Definition order should match declaration order, so you probably need to move > this method around. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:628: On 2013/07/03 17:07:01, vandebo wrote: > Should this also include a ProcSet entry? Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:634: content->writeText("/Pattern CS /Pattern cs /P0 SCN /P0 scn "); On 2013/07/03 17:07:01, vandebo wrote: > nit: omit extra whitespace. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:636: content->writeScalarAsText(bounds.left()); On 2013/07/03 17:07:01, vandebo wrote: > SkPDFUtils::AppendRectangle() > SkPDFUtils::PaintPath() Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:655: SkAutoTUnref<SkPDFObject> On 2013/07/03 17:07:01, vandebo wrote: > nit indent: > SkAutoTUnref<SkPDFObject> alphaShader(SkPDFShader::GetPDFShaderByState( > state->CreateAlphaToRGBState())); Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:665: SkAutoTUnref<SkData> alphaData(alphaContent->copyToData()); On 2013/07/03 17:07:01, vandebo wrote: > This mess is caused by incomplete conversion of SkStream to SkData from > SkStream. Can you do a pre-CL that finishes that conversion? Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:671: fAlphaGs.reset(SkPDFGraphicState::GetSMaskGraphicState(alphaMask.get(), false, false)); On 2013/07/03 17:07:01, vandebo wrote: > Not sure, but I think you want to pull this all out to a helper function/method > that take state as an argument and return the smask graphic state as a result. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:676: SkAutoTUnref<SkPDFArray> bboxArray(new SkPDFArray); On 2013/07/03 17:07:01, vandebo wrote: > Use rectToArray here as well. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:685: writePatternFillContent(colorContent.get(), bbox); On 2013/07/03 17:07:01, vandebo wrote: > Make this method return an SkDyrnamicMemoryWStream and take an option to emit > "/G0 gs" or not. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:691: insertName("Type", "Pattern"); On 2013/07/03 17:07:01, vandebo wrote: > Most of this can get pulled out and shared with > SkPDFImageShader::SkPDFImageShader() Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:812: SkPDFFunctionShader* SkPDFFunctionShader::CreateAlphaToRGBShader() { On 2013/07/03 17:07:01, vandebo wrote: > This isn't used, remove. D'oh. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1125: fBBox(other.fBBox) On 2013/07/03 17:07:01, vandebo wrote: > Also need to copy over fImageTileModes That seems to be for image shaders only, and that isn't handled currently. See SkPDFShader::State::operator==, where fImageTileModes is only compared when fType == SkShader::kNone_GradientType https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1130: // allocate new storage to prevent pointer aliasing On 2013/07/03 17:07:01, vandebo wrote: > nit: comment unnecessary. I would actually say that this piece of code is pretty confusing and its intent isn't obvious, but it's a moot point since breaking it out into a new function implicitly adds a comment now. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1131: fColorData.set(sk_malloc_throw( On 2013/07/03 17:07:01, vandebo wrote: > All this allocation is the same as the other constructor, but it into a shared > method. Done. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:1143: SkASSERT(fType != SkShader::kNone_GradientType); On 2013/07/03 17:07:01, vandebo wrote: > Put this after line 1126 Done.
https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFFormXObject.cpp#... src/pdf/SkPDFFormXObject.cpp:47: if (!device->initialTransform().isIdentity()) { On 2013/07/03 23:32:09, ducky wrote: > On 2013/07/03 17:07:01, vandebo wrote: > > In your uses of the new constructor, do you take into consideration the > > initialtransform? > > I don't think there's a way to specify the initial transform from just a content > stream. It just defaults to identity, right? > Unless you think it would be a good idea to include an initial transform matrix > in the constructor? Since SkGradientShader is implemented in terms of two SkFunctionShaders, which both take into account the initial transform, I think it works out ok. You probably should test with an initial transform though. It might be a bit cleaner/smaller to deal with the initial transform in the top level object and have the dependent objects not have to worry about it, but I suspect the code to make that happen would be more complicated. https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/1/src/pdf/SkPDFShader.cpp#newco... src/pdf/SkPDFShader.cpp:520: SkPDFObject* SkPDFShader::GetPDFShaderByState(State* inState) { On 2013/07/03 23:32:09, ducky wrote: > On 2013/07/03 17:07:01, vandebo wrote: > > Definition order should match declaration order, so you probably need to move > > this method around. > > Done. Not done https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:45: commonInit(NULL); nit: move to line 28 https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:64: void SkPDFFormXObject::commonInit(const char colorSpace[]) { pass as much as possible into init. i.e. bbox and resourceDict https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.h:54: void commonInit(const char colorSpace[]); nit: just init(...) https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:413: explicit State(const SkShader& shader, const SkMatrix& canvasTransform, nit: remove explicit https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:424: State(const State& other); The Rule of three says you should also declare an assignment operator method. It can be private and not defined. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:447: SkPDFFunctionShader* CreateAlphaToLuminosityShader(); unused https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:463: class SkPDFGradientShader : public SkPDFStream, public SkPDFShader { nit: There's probably a better name for this... FunctionShader is also a gradient shader. Not sure what the better name is, but it probably includes alpha...? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:533: // This is an internal method. CanonicalShadersMutex() should already be acquired. Also comment that this method takes ownership of |inState|. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:618: static SkPDFResourceDict* getGradientResourceDict( static helper functions are named with underscores and no capitals. nit: s/get/new/ https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:630: dict->insert("ProcSet", SkPDFUtils::CreateFullProcSetsArray())->unref(); Is this needed, I wasn't sure if it was. If it is, the full proc set certainly isn't needed; no text or images used here. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:635: static void populateTilingPatternDict(SkPDFDict* pattern, Here too and below... https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:655: static SkData* createPatternFillContent(bool setGs0, SkRect& bounds) { setGs0 feels odd. Maybe pass a SkString stream_setup ? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { I think if this function takes ownership of |luminosityShader| the caller will be a bit cleaner. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:673: SkAutoTUnref<SkPDFResourceDict> Move this to line 679. - Generally, define variables just before they are used. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:675: SkTSet<SkPDFObject*> resourcesArray; This isn't used... https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:678: SkAutoTUnref<SkData> alphaData(createPatternFillContent(false, bbox)); Event though this is of type SkData, I would call it alphaStream because it's the alpha content stream. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); Can you combine lines 700 and 701? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! nit: ! not needed, a . will suffice. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:449: class SkPDFResourceDict : public SkPDFDict { Not sure I'm happy with how this turned out. It's very heavy weight compared to an extra argument to the SkPDFFormXObject constructor. Would this type be useful in other parts of the existing code? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:453: /** Create a PDF dictionary. Maximum number of entries is 4095. comments are out of date....
https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:45: commonInit(NULL); On 2013/07/08 19:02:25, vandebo wrote: > nit: move to line 28 Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:64: void SkPDFFormXObject::commonInit(const char colorSpace[]) { On 2013/07/08 19:02:25, vandebo wrote: > pass as much as possible into init. i.e. bbox and resourceDict Done. Though, it's still not a completely clean solution, as bbox is now unref'd inside init (while being created outside init) and the different resourceDict styles mean different code exists to handle resources. Should the new resourceDict style work, this may become much cleaner. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.h:54: void commonInit(const char colorSpace[]); On 2013/07/08 19:02:25, vandebo wrote: > nit: just init(...) Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:413: explicit State(const SkShader& shader, const SkMatrix& canvasTransform, On 2013/07/08 19:02:25, vandebo wrote: > nit: remove explicit Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:424: State(const State& other); On 2013/07/08 19:02:25, vandebo wrote: > The Rule of three says you should also declare an assignment operator method. > It can be private and not defined. Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:447: SkPDFFunctionShader* CreateAlphaToLuminosityShader(); On 2013/07/08 19:02:25, vandebo wrote: > unused Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:463: class SkPDFGradientShader : public SkPDFStream, public SkPDFShader { On 2013/07/08 19:02:25, vandebo wrote: > nit: There's probably a better name for this... FunctionShader is also a > gradient shader. Not sure what the better name is, but it probably includes > alpha...? There probably is a better name. SkPDFAlphaGradientShader? That completely describes it, though it's a bit wordy... https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:533: // This is an internal method. CanonicalShadersMutex() should already be acquired. On 2013/07/08 19:02:25, vandebo wrote: > Also comment that this method takes ownership of |inState|. Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:618: static SkPDFResourceDict* getGradientResourceDict( On 2013/07/08 19:02:25, vandebo wrote: > static helper functions are named with underscores and no capitals. > nit: s/get/new/ Done. Though the functions generating postscript code on top don't conform to this... https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:630: dict->insert("ProcSet", SkPDFUtils::CreateFullProcSetsArray())->unref(); On 2013/07/08 19:02:25, vandebo wrote: > Is this needed, I wasn't sure if it was. If it is, the full proc set certainly > isn't needed; no text or images used here. ProcSet is no longer used in current versions of the PDF spec. However, the spec still recommends adding it only for backwards compatibility. I added the full set since it was how it was done in the only other instance ProcSet was defined. It's definitely overkill, but is it worth optimizing for something that shouldn't actually be used? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:635: static void populateTilingPatternDict(SkPDFDict* pattern, On 2013/07/08 19:02:25, vandebo wrote: > Here too and below... Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:655: static SkData* createPatternFillContent(bool setGs0, SkRect& bounds) { On 2013/07/08 19:02:25, vandebo wrote: > setGs0 feels odd. Maybe pass a SkString stream_setup ? Done. Yeah, that was ugly. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { On 2013/07/08 19:02:25, vandebo wrote: > I think if this function takes ownership of |luminosityShader| the caller will > be a bit cleaner. But doesn't that just shift the unref code from the caller to inside this function? Could it also be more confusing - in that unlike most other functions, this one will unref one of its arguments? I also kind of like having the creation and destruction in the same scope (where applicable). https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:673: SkAutoTUnref<SkPDFResourceDict> On 2013/07/08 19:02:25, vandebo wrote: > Move this to line 679. - Generally, define variables just before they are used. Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:675: SkTSet<SkPDFObject*> resourcesArray; On 2013/07/08 19:02:25, vandebo wrote: > This isn't used... Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:678: SkAutoTUnref<SkData> alphaData(createPatternFillContent(false, bbox)); On 2013/07/08 19:02:25, vandebo wrote: > Event though this is of type SkData, I would call it alphaStream because it's > the alpha content stream. Done, and similarly for colorStream below. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); On 2013/07/08 19:02:25, vandebo wrote: > Can you combine lines 700 and 701? Not unless the definition for setData is changed to return the SkData* passed in, in which case I could chain it with a unref. Incidentally, changing setData like that will also clean up some of the code for the SkPDFStream change CL. Otherwise, I can't think of a way to simultaneously set the data and unref the object after the data is set. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! On 2013/07/08 19:02:25, vandebo wrote: > nit: ! not needed, a . will suffice. Done. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:449: class SkPDFResourceDict : public SkPDFDict { On 2013/07/08 19:02:25, vandebo wrote: > Not sure I'm happy with how this turned out. It's very heavy weight compared to > an extra argument to the SkPDFFormXObject constructor. Would this type be > useful in other parts of the existing code? Looks like it can be used in SkPDFDevice, the only other place where a resource dict is constructed. It should make the resource list a bit more encapsulated and the construction slightly prettier and shorter at a slight cost in efficiency (to do the sub-dict lookup, though that can be heuristically sped-up by looking through the list in reverse order since similar resources tend to be added together). As for possible conflicts with the substitution code, I don't see any. While I don't fully understand the substitution code, it seems that most substitution happens in SkPDFCatalog and SkPDFStream. SkPDFDevice itself doesn't know anything about substitutes, and the code used to construct the resourceDict / getResources all rely on private variables fFontResources, fShaderResources, ... . As far as I can tell, code only adds entries, and no code modifies existing entries. Furthermore, it seems that getResourceDict memoizes the constructed dict while getResources pulls data from the current resource lists - so if there was any funny business going on, getResourceDict and getResources (which usually are called together) would return inconsistent results. Though, again, since I don't completely understand the substitution code, you may want to check my logic. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:453: /** Create a PDF dictionary. Maximum number of entries is 4095. On 2013/07/08 19:02:25, vandebo wrote: > comments are out of date.... Done.
https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:64: void SkPDFFormXObject::commonInit(const char colorSpace[]) { On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > pass as much as possible into init. i.e. bbox and resourceDict > > Done. Though, it's still not a completely clean solution, as bbox is now unref'd > inside init (while being created outside init) and the different resourceDict > styles mean different code exists to handle resources. Should the new > resourceDict style work, this may become much cleaner. As you pointed out at a different point, you probably want to own the object on the calling side. At the least, the ownership semantics should be documented and preferably be the same for all the arguments. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:463: class SkPDFGradientShader : public SkPDFStream, public SkPDFShader { On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > nit: There's probably a better name for this... FunctionShader is also a > > gradient shader. Not sure what the better name is, but it probably includes > > alpha...? > > There probably is a better name. SkPDFAlphaGradientShader? That completely > describes it, though it's a bit wordy... SkPDFFunctionShaderWithAlpha ? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:618: static SkPDFResourceDict* getGradientResourceDict( On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > static helper functions are named with underscores and no capitals. > > nit: s/get/new/ > > Done. Though the functions generating postscript code on top don't conform to > this... That was before the style guide was codified. You may fix those functions if you like; though it would be best to do it in a different CL. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:630: dict->insert("ProcSet", SkPDFUtils::CreateFullProcSetsArray())->unref(); On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > Is this needed, I wasn't sure if it was. If it is, the full proc set > certainly > > isn't needed; no text or images used here. > > ProcSet is no longer used in current versions of the PDF spec. However, the spec > still recommends adding it only for backwards compatibility. I added the full > set since it was how it was done in the only other instance ProcSet was defined. > It's definitely overkill, but is it worth optimizing for something that > shouldn't actually be used? Well, it's optimizing for size. Though I guess the number of copies of this is small enough that it doesn't matter. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > I think if this function takes ownership of |luminosityShader| the caller will > > be a bit cleaner. > > But doesn't that just shift the unref code from the caller to inside this > function? Could it also be more confusing - in that unlike most other functions, > this one will unref one of its arguments? I also kind of like having the > creation and destruction in the same scope (where applicable). Fair. What about making it a member of the class and passing in state instead? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > Can you combine lines 700 and 701? > > Not unless the definition for setData is changed to return the SkData* passed > in, in which case I could chain it with a unref. Fair. > Incidentally, changing setData like that will also clean up some of the code for > the SkPDFStream change CL. > Otherwise, I can't think of a way to simultaneously set the data and unref the > object after the data is set. Having setData return the new data is a bit strange (if it returned the old data, that might make some sense, but isn't useful). Lets leave it as it is. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! On 2013/07/09 02:56:23, ducky wrote: > On 2013/07/08 19:02:25, vandebo wrote: > > nit: ! not needed, a . will suffice. > > Done. Not done. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (left): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFDevice.cpp#o... src/pdf/SkPDFDevice.cpp:1137: SkPDFDict* SkPDFDevice::getResourceDict() { Should getResourceDict and getResources be combined to just return an SkPDFResourceDict? This can be done in a follow up CL. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:51: resourceDict->ref(); This ref is for the call to init, right? It should go there then. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:425: State operator=(const State& rhs) { return State(rhs); } I don't think you need an implementation. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:516: // This also takes ownership of inState, which is deleted on return. nix "which is..." If it takes ownership than the caller shouldn't expect to be able to access it anymore. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp#ne... src/pdf/SkPDFTypes.cpp:513: return insertResource(new SkPDFName(type), new SkPDFName(key), value); SkNew should be used for new uses of "new" https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:450: class SkPDFResourceDict : public SkPDFDict { This should go in its own file. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:456: SkPDFResourceDict() : SkPDFDict() {}; Should this take a parameter indicating whether it should include a procset entry? https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:461: explicit SkPDFResourceDict(const char type[]) : SkPDFDict(type) {}; Is this used/needed? https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:471: SkPDFObject* insertResource(SkPDFName* type, SkPDFName* key, |type| should probably be an enum. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:471: SkPDFObject* insertResource(SkPDFName* type, SkPDFName* key, |key| should probably be an int with class static constants defining the type specific prefix. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:494: SkPDFObject* insertResourceAsRef(const char type[], const char key[], Aside from proc set, shouldn't all resources in the resource dict be by reference? That means the above methods should go away.
SkPDFResourceDict moved into separate CL 18977002, and several more stylistic improvements. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:64: void SkPDFFormXObject::commonInit(const char colorSpace[]) { On 2013/07/09 17:47:44, vandebo wrote: > On 2013/07/09 02:56:23, ducky wrote: > > On 2013/07/08 19:02:25, vandebo wrote: > > > pass as much as possible into init. i.e. bbox and resourceDict > > > > Done. Though, it's still not a completely clean solution, as bbox is now > unref'd > > inside init (while being created outside init) and the different resourceDict > > styles mean different code exists to handle resources. Should the new > > resourceDict style work, this may become much cleaner. > > As you pointed out at a different point, you probably want to own the object on > the calling side. > > At the least, the ownership semantics should be documented and preferably be the > same for all the arguments. Fixed. Ownership is no longer transferred, and the caller is responsible for cleaning up. Two more lines of boilerplate, but should be more intuitive. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:463: class SkPDFGradientShader : public SkPDFStream, public SkPDFShader { On 2013/07/09 17:47:44, vandebo wrote: > On 2013/07/09 02:56:23, ducky wrote: > > On 2013/07/08 19:02:25, vandebo wrote: > > > nit: There's probably a better name for this... FunctionShader is also a > > > gradient shader. Not sure what the better name is, but it probably includes > > > alpha...? > > > > There probably is a better name. SkPDFAlphaGradientShader? That completely > > describes it, though it's a bit wordy... > > SkPDFFunctionShaderWithAlpha ? Also pretty long... how about SkPDFAlphaFunctionShader? https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { On 2013/07/09 17:47:44, vandebo wrote: > On 2013/07/09 02:56:23, ducky wrote: > > On 2013/07/08 19:02:25, vandebo wrote: > > > I think if this function takes ownership of |luminosityShader| the caller > will > > > be a bit cleaner. > > > > But doesn't that just shift the unref code from the caller to inside this > > function? Could it also be more confusing - in that unlike most other > functions, > > this one will unref one of its arguments? I also kind of like having the > > creation and destruction in the same scope (where applicable). > > Fair. What about making it a member of the class and passing in state instead? That also seems a bit inelegant, where you have one helper function pulled into the class, while the rest are static (since there's little reason to pull those in). Additionally, it just means moving that one line of SkAutoTUnref code into the function, so I don't see what we get? Furthermore, we have both shaders being constructed in the SkPDFGradientShader constructor, so it's kind of nice from a consistency standpoint. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); On 2013/07/09 17:47:44, vandebo wrote: > On 2013/07/09 02:56:23, ducky wrote: > > On 2013/07/08 19:02:25, vandebo wrote: > > > Can you combine lines 700 and 701? > > > > Not unless the definition for setData is changed to return the SkData* passed > > in, in which case I could chain it with a unref. > > Fair. > > > Incidentally, changing setData like that will also clean up some of the code > for > > the SkPDFStream change CL. > > Otherwise, I can't think of a way to simultaneously set the data and unref the > > object after the data is set. > > Having setData return the new data is a bit strange (if it returned the old > data, that might make some sense, but isn't useful). Lets leave it as it is. setData could return its input parameter (which incidentally is the new data, at least after the refactor CL). This seems to be the convention with SkPDFDict::insert, which returns the value being inserted, and is sometimes chained with unref() https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! On 2013/07/09 17:47:44, vandebo wrote: > On 2013/07/09 02:56:23, ducky wrote: > > On 2013/07/08 19:02:25, vandebo wrote: > > > nit: ! not needed, a . will suffice. > > > > Done. > > Not done. No, it is done. I think you're looking at an outdated version. Should be fixed as of Patch Set 5. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (left): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFDevice.cpp#o... src/pdf/SkPDFDevice.cpp:1137: SkPDFDict* SkPDFDevice::getResourceDict() { On 2013/07/09 17:47:44, vandebo wrote: > Should getResourceDict and getResources be combined to just return an > SkPDFResourceDict? This can be done in a follow up CL. Done. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:51: resourceDict->ref(); On 2013/07/09 17:47:44, vandebo wrote: > This ref is for the call to init, right? It should go there then. Done. The insert call should actually handle refing, so this would be extraneous anyways. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:425: State operator=(const State& rhs) { return State(rhs); } On 2013/07/09 17:47:44, vandebo wrote: > I don't think you need an implementation. Wait, wha? So I need an assignment operator, but it doesn't need to do anything? Why? And since I already have a copy operator, is there any reason why it shouldn't just reference it? https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:516: // This also takes ownership of inState, which is deleted on return. On 2013/07/09 17:47:44, vandebo wrote: > nix "which is..." If it takes ownership than the caller shouldn't expect to be > able to access it anymore. Done. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp#ne... src/pdf/SkPDFTypes.cpp:513: return insertResource(new SkPDFName(type), new SkPDFName(key), value); On 2013/07/09 17:47:44, vandebo wrote: > SkNew should be used for new uses of "new" Ok. I've changed it for this file. Does this apply to all new uses of new, or only those in files where SkNEW is the prevailing convention? There seems to be a good mix in the existing codebase... https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h File src/pdf/SkPDFTypes.h (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:450: class SkPDFResourceDict : public SkPDFDict { On 2013/07/09 17:47:44, vandebo wrote: > This should go in its own file. Done. Will also move to a separate CL. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:456: SkPDFResourceDict() : SkPDFDict() {}; On 2013/07/09 17:47:44, vandebo wrote: > Should this take a parameter indicating whether it should include a procset > entry? Done. Added a boolean parameter to automatically create the full set of ProcSets. Realistically, there shouldn't be a reason to have that set to false, but having it as an explicit parameter should make the background magic clearer from a readability standpoint. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:461: explicit SkPDFResourceDict(const char type[]) : SkPDFDict(type) {}; On 2013/07/09 17:47:44, vandebo wrote: > Is this used/needed? Looks like no, as the spec makes no mention of a Type entry in a resource dict. Removed. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:471: SkPDFObject* insertResource(SkPDFName* type, SkPDFName* key, On 2013/07/09 17:47:44, vandebo wrote: > |type| should probably be an enum. Done. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:471: SkPDFObject* insertResource(SkPDFName* type, SkPDFName* key, On 2013/07/09 17:47:44, vandebo wrote: > |type| should probably be an enum. Done. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.h#newc... src/pdf/SkPDFTypes.h:494: SkPDFObject* insertResourceAsRef(const char type[], const char key[], On 2013/07/09 17:47:44, vandebo wrote: > Aside from proc set, shouldn't all resources in the resource dict be by > reference? That means the above methods should go away. It doesn't have to be - I've seen PDFs where resource dicts contain an embedded object, rather than a reference to another object. I also think that the way these are implemented is fairly clean, so unless the insertResource(...) is broken (Skia can do embedded objects in dicts, right?), there isn't much of a reason to remove them?
https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:463: class SkPDFGradientShader : public SkPDFStream, public SkPDFShader { On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > On 2013/07/09 02:56:23, ducky wrote: > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > nit: There's probably a better name for this... FunctionShader is also a > > > > gradient shader. Not sure what the better name is, but it probably > includes > > > > alpha...? > > > > > > There probably is a better name. SkPDFAlphaGradientShader? That completely > > > describes it, though it's a bit wordy... > > > > SkPDFFunctionShaderWithAlpha ? > > Also pretty long... how about SkPDFAlphaFunctionShader? SGTM https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > On 2013/07/09 02:56:23, ducky wrote: > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > I think if this function takes ownership of |luminosityShader| the caller > > will > > > > be a bit cleaner. > > > > > > But doesn't that just shift the unref code from the caller to inside this > > > function? Could it also be more confusing - in that unlike most other > > functions, > > > this one will unref one of its arguments? I also kind of like having the > > > creation and destruction in the same scope (where applicable). > > > > Fair. What about making it a member of the class and passing in state instead? > > That also seems a bit inelegant, where you have one helper function pulled into > the class, while the rest are static (since there's little reason to pull those > in). Additionally, it just means moving that one line of SkAutoTUnref code into > the function, so I don't see what we get? Furthermore, we have both shaders > being constructed in the SkPDFGradientShader constructor, so it's kind of nice > from a consistency standpoint. You're only thinking of it as a helper function because that's where it is now. By doing it, you get better encapsulation of the functionality of this method. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > On 2013/07/09 02:56:23, ducky wrote: > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > Can you combine lines 700 and 701? > > > > > > Not unless the definition for setData is changed to return the SkData* > passed > > > in, in which case I could chain it with a unref. > > > > Fair. > > > > > Incidentally, changing setData like that will also clean up some of the code > > for > > > the SkPDFStream change CL. > > > Otherwise, I can't think of a way to simultaneously set the data and unref > the > > > object after the data is set. > > > > Having setData return the new data is a bit strange (if it returned the old > > data, that might make some sense, but isn't useful). Lets leave it as it is. > > setData could return its input parameter (which incidentally is the new data, at > least after the refactor CL). This seems to be the convention with > SkPDFDict::insert, which returns the value being inserted, and is sometimes > chained with unref() > If you like. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > On 2013/07/09 02:56:23, ducky wrote: > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > nit: ! not needed, a . will suffice. > > > > > > Done. > > > > Not done. > > No, it is done. I think you're looking at an outdated version. Should be fixed > as of Patch Set 5. In patch set 5 (and 7) line 1120 (1123) has a ! https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:425: State operator=(const State& rhs) { return State(rhs); } On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > I don't think you need an implementation. > > Wait, wha? > So I need an assignment operator, but it doesn't need to do anything? Why? > And since I already have a copy operator, is there any reason why it shouldn't > just reference it? See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... We don't want an empty implementation, but no implementation at all. If anyone ever tries to use it, the linker will complain about it not existing. You do have a copy constructor, but it's private and used for a specific implementation detail. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp File src/pdf/SkPDFTypes.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFTypes.cpp#ne... src/pdf/SkPDFTypes.cpp:513: return insertResource(new SkPDFName(type), new SkPDFName(key), value); On 2013/07/10 21:42:26, ducky wrote: > On 2013/07/09 17:47:44, vandebo wrote: > > SkNew should be used for new uses of "new" > > Ok. I've changed it for this file. Does this apply to all new uses of new, or > only those in files where SkNEW is the prevailing convention? There seems to be > a good mix in the existing codebase... All new uses https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:59: init("DeviceRGB", resourceDict, bboxArray); Is CS required here? I think table 7.13 of the 1.4 pdf spec implies it is. Is that your reading as well? After this goes in, we might want to look into how much work it would be to generate a DeviceGray FormXObject and use that instead. https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.h:55: void init(const char colorSpace[], nit: const char* colorSpace. IIRC, once you aren't in the original declaration scope, the difference doesn't gain you anything. https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFUtils.h File src/pdf/SkPDFUtils.h (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFUtils.h#newc... src/pdf/SkPDFUtils.h:41: static SkPDFArray* CreateFullProcSetsArray(); Remove
Fixed code review comments, along with a little more cleanup. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:672: static SkPDFGraphicState* createSMaskGraphicState(SkPDFObject* luminosityShader, SkRect& bbox) { Ah, I see. Done. On 2013/07/11 22:22:23, vandebo wrote: > On 2013/07/10 21:42:26, ducky wrote: > > On 2013/07/09 17:47:44, vandebo wrote: > > > On 2013/07/09 02:56:23, ducky wrote: > > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > > I think if this function takes ownership of |luminosityShader| the > caller > > > will > > > > > be a bit cleaner. > > > > > > > > But doesn't that just shift the unref code from the caller to inside this > > > > function? Could it also be more confusing - in that unlike most other > > > functions, > > > > this one will unref one of its arguments? I also kind of like having the > > > > creation and destruction in the same scope (where applicable). > > > > > > Fair. What about making it a member of the class and passing in state > instead? > > > > That also seems a bit inelegant, where you have one helper function pulled > into > > the class, while the rest are static (since there's little reason to pull > those > > in). Additionally, it just means moving that one line of SkAutoTUnref code > into > > the function, so I don't see what we get? Furthermore, we have both shaders > > being constructed in the SkPDFGradientShader constructor, so it's kind of nice > > from a consistency standpoint. > > You're only thinking of it as a helper function because that's where it is now. > By doing it, you get better encapsulation of the functionality of this method. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:701: setData(colorData.get()); On 2013/07/11 22:22:23, vandebo wrote: > On 2013/07/10 21:42:26, ducky wrote: > > On 2013/07/09 17:47:44, vandebo wrote: > > > On 2013/07/09 02:56:23, ducky wrote: > > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > > Can you combine lines 700 and 701? > > > > > > > > Not unless the definition for setData is changed to return the SkData* > > passed > > > > in, in which case I could chain it with a unref. > > > > > > Fair. > > > > > > > Incidentally, changing setData like that will also clean up some of the > code > > > for > > > > the SkPDFStream change CL. > > > > Otherwise, I can't think of a way to simultaneously set the data and unref > > the > > > > object after the data is set. > > > > > > Having setData return the new data is a bit strange (if it returned the old > > > data, that might make some sense, but isn't useful). Lets leave it as it is. > > > > setData could return its input parameter (which incidentally is the new data, > at > > least after the refactor CL). This seems to be the convention with > > SkPDFDict::insert, which returns the value being inserted, and is sometimes > > chained with unref() > > > > If you like. Actually, I think I'm going to leave this as-is. If it becomes worthwhile later, it probably should get its own refactor CL, where things become consistent across the board. https://codereview.chromium.org/18585002/diff/23001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:1121: * Only valid for gradient states! On 2013/07/11 22:22:23, vandebo wrote: > On 2013/07/10 21:42:26, ducky wrote: > > On 2013/07/09 17:47:44, vandebo wrote: > > > On 2013/07/09 02:56:23, ducky wrote: > > > > On 2013/07/08 19:02:25, vandebo wrote: > > > > > nit: ! not needed, a . will suffice. > > > > > > > > Done. > > > > > > Not done. > > > > No, it is done. I think you're looking at an outdated version. Should be fixed > > as of Patch Set 5. > > In patch set 5 (and 7) line 1120 (1123) has a ! I see. There were two. D'oh. https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:425: State operator=(const State& rhs) { return State(rhs); } On 2013/07/11 22:22:23, vandebo wrote: > On 2013/07/10 21:42:26, ducky wrote: > > On 2013/07/09 17:47:44, vandebo wrote: > > > I don't think you need an implementation. > > > > Wait, wha? > > So I need an assignment operator, but it doesn't need to do anything? Why? > > And since I already have a copy operator, is there any reason why it shouldn't > > just reference it? > > See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... > > We don't want an empty implementation, but no implementation at all. If anyone > ever tries to use it, the linker will complain about it not existing. > > You do have a copy constructor, but it's private and used for a specific > implementation detail. Done. https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:59: init("DeviceRGB", resourceDict, bboxArray); On 2013/07/11 22:22:23, vandebo wrote: > Is CS required here? I think table 7.13 of the 1.4 pdf spec implies it is. Is > that your reading as well? > > After this goes in, we might want to look into how much work it would be to > generate a DeviceGray FormXObject and use that instead. Yep, looks like it's required when in luminosity mode. I also tried omitting it, and the results were not as expected. As for DeviceGray, I just tried and it works with both poppler and Chrome. I'm going to leave it as DeviceRGB for now, since while it works, it may not be spec-compliant. A more detailed look at the spec will be required (later). The current gradient code outputs RGB, right? https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.h File src/pdf/SkPDFFormXObject.h (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.h:55: void init(const char colorSpace[], On 2013/07/11 22:22:23, vandebo wrote: > nit: const char* colorSpace. IIRC, once you aren't in the original declaration > scope, the difference doesn't gain you anything. Done. I looked this up (out of curiosity), and it turns out that both forms are equivalent, and [] parameters are degraded into pointers. https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFUtils.h File src/pdf/SkPDFUtils.h (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFUtils.h#newc... src/pdf/SkPDFUtils.h:41: static SkPDFArray* CreateFullProcSetsArray(); On 2013/07/11 22:22:23, vandebo wrote: > Remove Done.
a few nits... https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.cpp File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/47002/src/pdf/SkPDFFormXObject.... src/pdf/SkPDFFormXObject.cpp:59: init("DeviceRGB", resourceDict, bboxArray); On 2013/07/12 03:40:00, ducky wrote: > On 2013/07/11 22:22:23, vandebo wrote: > > Is CS required here? I think table 7.13 of the 1.4 pdf spec implies it is. > Is > > that your reading as well? > > > > After this goes in, we might want to look into how much work it would be to > > generate a DeviceGray FormXObject and use that instead. > > Yep, looks like it's required when in luminosity mode. I also tried omitting it, > and the results were not as expected. > > As for DeviceGray, I just tried and it works with both poppler and Chrome. I'm > going to leave it as DeviceRGB for now, since while it works, it may not be > spec-compliant. A more detailed look at the spec will be required (later). The > current gradient code outputs RGB, right? Converting to DeviceGray would require changing the gradient code to output just a single channel. It may work, but it's probably detecting the mismatch and ignoring the DeviceGray color space. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:677: SkPDFGraphicState* SkPDFAlphaFunctionShader::CreateSMaskGraphicState( Instead of passing in state, use fState. Instead of passing in bbox, use fState.get()->fBBox https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:679: SkAutoTUnref<SkPDFObject> luminosityShader(SkPDFShader::GetPDFShaderByState( This might be easier to read as: SkAutoTUnref<SkPDFObject> luminosityShader( SkPDFShader::GetPDFShaderByState( alphaState->CreateAlphaToLuminosityState())); or as mentioned elsewhere: SkAutoTUnref<State> alphaState(state->CreateAlphaToLuminosityState()); SkAutoTUnref<SkPDFObject> luminosityShader( SkPDFShader::GetPDFShaderByState(alphaState)); https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:696: fState(state) { nit: : fState(state) { https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:700: fColorPattern.reset( nit: Maybe fColorShader would be a better name, not sure. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:710: create_pattern_fill_content("/G0 gs", bbox)); you'll need to rebase this on the recent resource dict changes. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.h File src/pdf/SkPDFShader.h (right): https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.h#new... src/pdf/SkPDFShader.h:61: static SkPDFObject* GetPDFShaderByState(State* shaderState); At the least this should document that it consumes the state that's passed in. But it'd probably be better if it didn't - that may be counter to what I've said before, sorry if so.
https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:677: SkPDFGraphicState* SkPDFAlphaFunctionShader::CreateSMaskGraphicState( On 2013/07/12 21:41:44, vandebo wrote: > Instead of passing in state, use fState. > Instead of passing in bbox, use fState.get()->fBBox Done. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:679: SkAutoTUnref<SkPDFObject> luminosityShader(SkPDFShader::GetPDFShaderByState( On 2013/07/12 21:41:44, vandebo wrote: > This might be easier to read as: > SkAutoTUnref<SkPDFObject> luminosityShader( > SkPDFShader::GetPDFShaderByState( > alphaState->CreateAlphaToLuminosityState())); > > or as mentioned elsewhere: > > SkAutoTUnref<State> alphaState(state->CreateAlphaToLuminosityState()); > SkAutoTUnref<SkPDFObject> luminosityShader( > SkPDFShader::GetPDFShaderByState(alphaState)); Done. Using first since there are issues with the second. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:696: fState(state) { On 2013/07/12 21:41:44, vandebo wrote: > nit: : fState(state) { Done. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:700: fColorPattern.reset( On 2013/07/12 21:41:44, vandebo wrote: > nit: Maybe fColorShader would be a better name, not sure. Done. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:710: create_pattern_fill_content("/G0 gs", bbox)); On 2013/07/12 21:41:44, vandebo wrote: > you'll need to rebase this on the recent resource dict changes. Done. https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.h File src/pdf/SkPDFShader.h (right): https://codereview.chromium.org/18585002/diff/31002/src/pdf/SkPDFShader.h#new... src/pdf/SkPDFShader.h:61: static SkPDFObject* GetPDFShaderByState(State* shaderState); On 2013/07/12 21:41:44, vandebo wrote: > At the least this should document that it consumes the state that's passed in. > But it'd probably be better if it didn't - that may be counter to what I've said > before, sorry if so. It would be cleaner if it didn't, but it needs to be able to transfer ownership of the shaderState from GetPDFShaderByState(...) to the newly created SkPDFShader. Otherwise, either: - each SkPDFShader instantiation would need to copy the state, which is probably bad from a performance standpoint and having extra code. - or SkPDFShader::State would need to be reference-counted, so that an additional ref() can be done if it is part of a new SkPDFShader object. Thoughts?
just waiting on depe https://codereview.chromium.org/18585002/diff/81001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/81001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:712: SkString setGs = SkString(); nit: This has changed enough that I think the original way is now cleaner.. pass a gs_index to create_pattern_fill_content and use SkPDFUtils::ApplyGraphicState(). Use a -1 index for no GS.
Clean-up for create_pattern_fill_content and deduplication of apply pattern content code. https://codereview.chromium.org/18585002/diff/81001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/81001/src/pdf/SkPDFShader.cpp#n... src/pdf/SkPDFShader.cpp:712: SkString setGs = SkString(); On 2013/07/15 16:40:06, vandebo wrote: > nit: This has changed enough that I think the original way is now cleaner.. pass > a gs_index to create_pattern_fill_content and use > SkPDFUtils::ApplyGraphicState(). Use a -1 index for no GS. Done.
https://codereview.chromium.org/18585002/diff/104001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/104001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:655: static SkData* create_pattern_fill_content(int applyGs, SkRect& bounds) { nit: applyGS -> GSIndex
Fixes code review comments. Also rebase with new SkPDFStream-SkData code. https://codereview.chromium.org/18585002/diff/104001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/104001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:655: static SkData* create_pattern_fill_content(int applyGs, SkRect& bounds) { On 2013/07/15 18:11:40, vandebo wrote: > nit: applyGS -> GSIndex Done.
https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFFormXObject... File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFFormXObject... src/pdf/SkPDFFormXObject.cpp:79: group->insertName("CS", colorSpace); ColorSpace is required. Also I think we should use the long name. BTW, can you point me in documentation where it lists CS as abbreviation for ColorSpace (I believe you , I just was not able to find it) see TABLE 4.35 Additional entries specific to an image dictionary ColorSpace name or (Required except for image masks; not allowed for image masks) The color array space in which image samples are specified. This may be any type of color space except Pattern. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFGraphicState.h File src/pdf/SkPDFGraphicState.h (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFGraphicStat... src/pdf/SkPDFGraphicState.h:63: static SkPDFGraphicState* GetSMaskGraphicState(SkPDFFormXObject* sMask, Nit: not your problem, since the code was like this, but Get_Foo() implies getting something, not making something, especially with an extra parameter, it is confusing. I recommend to rename Get with Make, which would be better name for what the function actually does https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:414: const SkIRect& bbox); bad alignment after removing explicit on previous line https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:547: new SkPDFAlphaFunctionShader(shaderState.detach()); nit: use SkNEW_ARGS https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:552: new SkPDFFunctionShader(shaderState.detach()); nit: use SkNEW_ARGS https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:580: return GetPDFShaderByState(new State(shader, matrix, surfaceBBox)); SkNEW_ARGS https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:638: pattern->insertInt("PatternType", 1); Nit: create constants out of these numbers, or comment them https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:660: SkPDFUtils::ApplyPattern(0, &content); constant 0, or comment what 0 means https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:706: create_pattern_fill_content(0, bbox)); constant 0, or comment what 0 means https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:1168: SkAlpha opaque = SkColorGetA(SK_ColorBLACK); define a new constant, instead of reeling on what the Alpha property of black color is https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp#n... src/pdf/SkPDFUtils.cpp:242: content->writeText("/Pattern CS/Pattern cs/"); Do you set the value of /Pattern twice? Per spec, In PDF world means that now /Pattern is undefined https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp#n... src/pdf/SkPDFUtils.cpp:244: content->writeText(" SCN/"); please comment the scn/SCN related lines
Style corrections based on review and more comments. What's the preferred way to define an opaque alpha? Should SkColor.h be modified (where the other colors are defined - and add #define Sk_AlphaOPAQUE 0xff), or should modifying the core classes be avoided? https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFFormXObject... File src/pdf/SkPDFFormXObject.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFFormXObject... src/pdf/SkPDFFormXObject.cpp:79: group->insertName("CS", colorSpace); PDF reference, page 540, table 7.13. It specifies CS as the entry for a transparency group. I don't think it will accept ColorSpace (or according to a strict, limited reading of the spec, it shouldn't). It's interesting how the spec is inconsistent between sections about doing this... On 2013/07/17 17:52:24, edisonn wrote: > ColorSpace is required. Also I think we should use the long name. BTW, can you > point me in documentation where it lists CS as abbreviation for ColorSpace (I > believe you , I just was not able to find it) > > > see TABLE 4.35 Additional entries specific to an image dictionary > > ColorSpace name or > (Required except for image masks; not allowed for image > masks) The color > array > space in which image samples are specified. This may be any > type of color > > space except Pattern. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFGraphicState.h File src/pdf/SkPDFGraphicState.h (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFGraphicStat... src/pdf/SkPDFGraphicState.h:63: static SkPDFGraphicState* GetSMaskGraphicState(SkPDFFormXObject* sMask, Hm - I think get_* is often used in a way that's similar to return_*, rather than literally get-a-stored-value. As for changing this to make, I think that, while it would make what it's doing clearer, it would make the class as a whole inelegant - you have similar functions like GetNoSMaskGraphicState(...) and GetInvertFunction(), all of them which also create (though also cache) objects. Renaming this one to Make... could make it appear as there is something significantly different about this one, even though it has the same refing semantics as the rest. GetSMaskGraphicState could also be written in a way where it's cached, though the current argument against that was it's unlikely you'd reuse a SMask. With gradients, that may change. On 2013/07/17 17:52:24, edisonn wrote: > Nit: not your problem, since the code was like this, but Get_Foo() implies > getting something, not making something, especially with an extra parameter, it > is confusing. > > I recommend to rename Get with Make, which would be better name for what the > function actually does https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp File src/pdf/SkPDFShader.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:414: const SkIRect& bbox); On 2013/07/17 17:52:24, edisonn wrote: > bad alignment after removing explicit on previous line Done. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:547: new SkPDFAlphaFunctionShader(shaderState.detach()); On 2013/07/17 17:52:24, edisonn wrote: > nit: use SkNEW_ARGS Done. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:552: new SkPDFFunctionShader(shaderState.detach()); On 2013/07/17 17:52:24, edisonn wrote: > nit: use SkNEW_ARGS Done. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:580: return GetPDFShaderByState(new State(shader, matrix, surfaceBBox)); On 2013/07/17 17:52:24, edisonn wrote: > SkNEW_ARGS Done. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:638: pattern->insertInt("PatternType", 1); On 2013/07/17 17:52:24, edisonn wrote: > Nit: create constants out of these numbers, or comment them Done. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:660: SkPDFUtils::ApplyPattern(0, &content); It's part of the function comments, which says this fills P0. Having a magic number isn't pretty, but the 0 doesn't really mean anything, other than the arbitrary resource number I chose to use. On 2013/07/17 17:52:24, edisonn wrote: > constant 0, or comment what 0 means https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:706: create_pattern_fill_content(0, bbox)); On 2013/07/17 17:52:24, edisonn wrote: > constant 0, or comment what 0 means Another resource index, arbitrarily assigned. Right now, the code basically has graphic state 0 and pattern 0 sprinkled around everywhere. I'll comment it. https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFShader.cpp#... src/pdf/SkPDFShader.cpp:1168: SkAlpha opaque = SkColorGetA(SK_ColorBLACK); On 2013/07/17 17:52:24, edisonn wrote: > define a new constant, instead of reeling on what the Alpha property of black > color is Best way to do this? #define SK_AlphaOPAQUE 0xff in SkColor.h? https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp File src/pdf/SkPDFUtils.cpp (right): https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp#n... src/pdf/SkPDFUtils.cpp:242: content->writeText("/Pattern CS/Pattern cs/"); On 2013/07/17 17:52:24, edisonn wrote: > Do you set the value of /Pattern twice? Per spec, In PDF world means that now > /Pattern is undefined Addressed in comment in the other CL (resource dicts). Postfix notation is kind of odd... https://codereview.chromium.org/18585002/diff/108001/src/pdf/SkPDFUtils.cpp#n... src/pdf/SkPDFUtils.cpp:244: content->writeText(" SCN/"); On 2013/07/17 17:52:24, edisonn wrote: > please comment the scn/SCN related lines Done.
This reverts the SkPDFStream to SkData CL and uses SkStream::detachAsStream() instead. Also uses the SK_AlphaOPAQUE constants in SkColor. Since https://codereview.chromium.org/19677002/ is in yet another update cycle, I'm using the currently-compiling detatchAsStream naming. I'll update this CL as soon as that one goes in.
LGTM. edison, are you happy?
Update method names from new SkPDFResourceDict
Still LGTM
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/18585002/154001
Message was sent while issue was closed.
Change committed as 10297
Message was sent while issue was closed.
On 2013/07/23 23:16:15, I haz the power (commit-bot) wrote: > Change committed as 10297 Mac GMs are failing after this; radial_gradient_pdf.png & gradtext_pdf.png may need rebaselining, please take a look. ['python', 'gm/display_json_results.py', '/Users/chrome-bot/skia-buildbot/skiabot-macmini-10_7-001/buildbot/third_party/chromium_buildbot/slave/Test-Mac10_7-MacMini4_1-GeForce320M-x86-Release/gm/actual/base-macmini-lion-float/actual-results.json'] [19:51:07.595274] [ ] 0 IgnoredExpectationsMismatch: [19:51:07.595748] [*] 2 ExpectationsMismatch: radial_gradient_pdf.png gradtext_pdf.png [19:51:07.595974] [ ] 446 MissingExpectations [19:51:07.596113] [ ] 679 Passed [19:51:07.596252] (results marked with [*] will cause nonzero return value) http://108.170.217.252:10115/builders/Test-Mac10.7-MacMini4.1-GeForce320M-x86...
Message was sent while issue was closed.
Ping - still seeing Mac GM failures for radial_gradient_pdf.png & gradtext_pdf.png.
Message was sent while issue was closed.
On 2013/07/24 22:12:14, fmalita wrote: > Ping - still seeing Mac GM failures for radial_gradient_pdf.png & > gradtext_pdf.png. Looks like my reply emails don't actually get attached to the CL, nor were you included in the reply-to field... Anyways, those are expected to be "failing", in that those are now fixed from the previous incorrect baselines. alphagradients_pdf.png is also fixed, and modecolorfilters_pdf.png has changed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
