Closed Bug 341564 Opened 19 years ago Closed 18 years ago

Consolidate repeated code in nsSVGFilters.cpp

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: malex, Assigned: malex)

Details

Attachments

(1 file, 7 obsolete files)

final patch (hopefully)
21.18 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060614 Minefield/3.0a1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060614 Minefield/3.0a1 Three of the filter functions in nsSVGFilters.cpp begin and end with the same code. The upcoming patch consolidates this code into helper functions. Reproducible: Always
Attached patch Patch (obsolete) — — Splinter Review
Consolidates code
Assignee: general → amenzie
The code in this patch can also be used to consolidate the nsSVGFEFloodElement::Filter function proposed by the patch for bug #211069
Oops, that should be bug #326143, Sorry!
Attachment #225621 - Flags: superreview?(tor)
Attachment #225621 - Flags: review?(longsonr)
Comment on attachment 225621 [details] [diff] [review] Patch As I said in one of your other bugs, I'm not a peer or the module owner for SVG so I can't formally review your code without some kind of authorisation from the module owner i.e. tor. Additionally I don't know this code very well. Tor: Are you intending to check in your additional filters patch Alex referred to? This fix could then address all the commonality in one go rather than bitrotting that patch. Nevertheless if I was able to review it, this is what I'd say... >+ >+static void >+fixupTarget(PRUint8 *aSource, PRUint8 *aTarget, >+ PRUint32 aWidth, PRUint32 aHeight, >+ PRInt32 aStride, nsRect aRegion); >+ >+NS_IMETHODIMP >+nsSVGFE::ObtainResources(nsSVGFilterInstance* aInstance, >+ nsCOMPtr<nsIDOMSVGAnimatedString> aIn1, >+ nsSVGFilterResource* aResource) >+{ >+ nsRect defaultRect; >+ aIn1->GetAnimVal(aResource->input); >+ mResult->GetAnimVal(aResource->result); >+ aInstance->LookupRegion(aResource->input, &defaultRect); >+ >+ aInstance->GetFilterSubregion(this, defaultRect, &aResource->rect); >+ aInstance->LookupImage(aResource->input, >+ getter_AddRefs(aResource->sourceImage)); >+ if (!aResource->sourceImage) >+ return NS_ERROR_FAILURE; >+ >+ aInstance->GetImage(getter_AddRefs(aResource->targetImage)); >+ if (!aResource->targetImage) >+ return NS_ERROR_FAILURE; >+ >+ aResource->sourceImage->Lock(); >+ aResource->targetImage->Lock(); >+ aResource->sourceImage->GetData(&aResource->sourceData, >+ &aResource->length, >+ &aResource->stride); >+ aResource->targetImage->GetData(&aResource->targetData, >+ &aResource->length, >+ &aResource->stride); >+ >+ aResource->sourceImage->GetWidth(&aResource->width); >+ aResource->sourceImage->GetHeight(&aResource->height); >+ return NS_OK; >+} Perhaps better as a member function of your nsSVGFilterResource class. I wonder if you could you could move the gaussianBlur/fixupTarget functions into nsSVGFilterResource too? You could then look at which, if any members, of nsSVGFilterResource should be public and possibly providing accessor methods for whatever needs to be accessed outside of nsSVGFilterResource rather than having any public variables. >+ >+void >+nsSVGFE::ReleaseResources(nsSVGFilterInstance* aInstance, >+ nsSVGFilterResource* aResource) >+{ >+ fixupTarget(aResource->sourceData, aResource->targetData, >+ aResource->width, aResource->height, >+ aResource->stride, aResource->rect); >+ >+ aResource->sourceImage->Unlock(); >+ aResource->targetImage->Unlock(); >+ >+ aInstance->DefineImage(aResource->result, aResource->targetImage); >+ aInstance->DefineRegion(aResource->result, aResource->rect); >+} Member function(s) of nsSVGFilterResource? or better as just code in its destructor. > > //---------------------------------------------------------------------- > // nsIDOMSVGFilterPrimitiveStandardAttributes methods > > /* readonly attribute nsIDOMSVGAnimatedLength x; */ > NS_IMETHODIMP nsSVGFE::GetX(nsIDOMSVGAnimatedLength * *aX) > { > return mLengthAttributes[X].ToDOMAnimatedLength(aX, this); >@@ -484,73 +538,39 @@ fixupTarget(PRUint8 *aSource, PRUint8 *a > memset(aTarget + y * aStride + 4 * (aRegion.x + aRegion.width), > 0, > 4 * (aWidth - aRegion.x - aRegion.width)); > } > > NS_IMETHODIMP > nsSVGFEGaussianBlurElement::Filter(nsSVGFilterInstance *instance) > { >- nsRect rect; >- nsIDOMSVGFilterPrimitiveStandardAttributes *attrib = >- (nsIDOMSVGFilterPrimitiveStandardAttributes *)this; >- >- nsRect defaultRect; >- nsAutoString input, result; >- mIn1->GetAnimVal(input); >- mResult->GetAnimVal(result); >- instance->LookupRegion(input, &defaultRect); >- >- instance->GetFilterSubregion(this, defaultRect, &rect); >+ nsSVGFilterResource res; >+ nsresult rv = ObtainResources(instance, mIn1, &res); >+ NS_ENSURE_SUCCESS(rv,rv); nit: space after comma. > > #ifdef DEBUG_tor > fprintf(stderr, "FILTER GAUSS rect: %d,%d %dx%d\n", > rect.x, rect.y, rect.width, rect.height); > #endif > >- nsCOMPtr<nsISVGRendererSurface> sourceImage, targetImage; >- instance->LookupImage(input, getter_AddRefs(sourceImage)); >- if (!sourceImage) >- return NS_ERROR_FAILURE; >- >- instance->GetImage(getter_AddRefs(targetImage)); >- if (!targetImage) >- return NS_ERROR_FAILURE; >- >- PRUint8 *sourceData, *targetData; >- PRInt32 stride; >- PRUint32 width, height, length; >- sourceImage->Lock(); >- targetImage->Lock(); >- sourceImage->GetData(&sourceData, &length, &stride); >- targetImage->GetData(&targetData, &length, &stride); >- >- sourceImage->GetWidth(&width); >- sourceImage->GetHeight(&height); >- > float stdX, stdY; > nsSVGLength2 val; > > mStdDeviationX->GetAnimVal(&stdX); > val.Init(nsSVGUtils::X, 0xff, stdX, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); > stdX = instance->GetPrimitiveLength(&val); > > mStdDeviationY->GetAnimVal(&stdY); > val.Init(nsSVGUtils::Y, 0xff, stdY, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); > stdY = instance->GetPrimitiveLength(&val); > >- gaussianBlur(sourceData, targetData, length, stride, rect, stdX, stdY); >- fixupTarget(sourceData, targetData, width, height, stride, rect); >- >- sourceImage->Unlock(); >- targetImage->Unlock(); >- >- instance->DefineImage(result, targetImage); >- instance->DefineRegion(result, rect); >- >+ gaussianBlur(res.sourceData, res.targetData, res.length, >+ res.stride, res.rect, stdX, stdY); >+ ReleaseResources(instance, &res); > return NS_OK; > } > > static PRUint32 > CheckStandardNames(nsIDOMSVGAnimatedString *aStr) > { > nsAutoString str; > aStr->GetAnimVal(str); >@@ -679,53 +699,25 @@ nsSVGFEComponentTransferElement::GetIn1( > *aIn = mIn1; > NS_IF_ADDREF(*aIn); > return NS_OK; > } > > NS_IMETHODIMP > nsSVGFEComponentTransferElement::Filter(nsSVGFilterInstance *instance) > { >- nsRect rect; >- nsIDOMSVGFilterPrimitiveStandardAttributes *attrib = >- (nsIDOMSVGFilterPrimitiveStandardAttributes *)this; >- >- nsRect defaultRect; >- nsAutoString input, result; >- mIn1->GetAnimVal(input); >- mResult->GetAnimVal(result); >- instance->LookupRegion(input, &defaultRect); >- >- instance->GetFilterSubregion(this, defaultRect, &rect); >+ nsSVGFilterResource res; >+ nsresult rv = ObtainResources(instance, mIn1, &res); >+ NS_ENSURE_SUCCESS(rv,rv); nit: space after comma. > > #ifdef DEBUG_tor > fprintf(stderr, "FILTER COMPONENT rect: %d,%d %dx%d\n", > rect.x, rect.y, rect.width, rect.height); > #endif > >- nsCOMPtr<nsISVGRendererSurface> sourceImage, targetImage; >- instance->LookupImage(input, getter_AddRefs(sourceImage)); >- if (!sourceImage) >- return NS_ERROR_FAILURE; >- >- instance->GetImage(getter_AddRefs(targetImage)); >- if (!targetImage) >- return NS_ERROR_FAILURE; >- >- PRUint8 *sourceData, *targetData; >- PRInt32 stride; >- PRUint32 width, height, length; >- sourceImage->Lock(); >- targetImage->Lock(); >- sourceImage->GetData(&sourceData, &length, &stride); >- targetImage->GetData(&targetData, &length, &stride); >- >- sourceImage->GetWidth(&width); >- sourceImage->GetHeight(&height); >- > PRUint8 tableR[256], tableG[256], tableB[256], tableA[256]; > for (int i=0; i<256; i++) > tableR[i] = tableG[i] = tableB[i] = tableA[i] = i; > PRUint32 count = GetChildCount(); > for (PRUint32 k = 0; k < count; k++) { > nsCOMPtr<nsIContent> child = GetChildAt(k); > nsCOMPtr<nsIDOMSVGFEFuncRElement> elementR = do_QueryInterface(child); > nsCOMPtr<nsIDOMSVGFEFuncGElement> elementG = do_QueryInterface(child); >@@ -736,41 +728,37 @@ nsSVGFEComponentTransferElement::Filter( > if (elementG) > elementG->GenerateLookupTable(tableG); > if (elementB) > elementB->GenerateLookupTable(tableB); > if (elementA) > elementA->GenerateLookupTable(tableA); > } > >- for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) >- for (PRInt32 x = rect.x; x < rect.x + rect.width; x++) { >+ for (PRInt32 y = res.rect.y; y < res.rect.y + res.rect.height; y++) >+ for (PRInt32 x = res.rect.x; x < res.rect.x + res.rect.width; x++) { > PRUint32 r, g, b, a; >- a = sourceData[y * stride + 4 * x + 3]; >+ a = res.sourceData[y * res.stride + 4 * x + 3]; > if (a) { >- b = tableB[(255 * (PRUint32)sourceData[y * stride + 4 * x]) / a]; >- g = tableG[(255 * (PRUint32)sourceData[y * stride + 4 * x + 1]) / a]; >- r = tableR[(255 * (PRUint32)sourceData[y * stride + 4 * x + 2]) / a]; >+ b = tableB[(255 * >+ (PRUint32)res.sourceData[y * res.stride + 4 * x]) / a]; >+ g = tableG[(255 * >+ (PRUint32)res.sourceData[y * res.stride + 4 * x + 1]) / a]; >+ r = tableR[(255 * >+ (PRUint32)res.sourceData[y * res.stride + 4 * x + 2]) / a]; > } else > b = g = r = 0; > a = tableA[a]; >- targetData[y * stride + 4 * x] = (b * a) / 255; >- targetData[y * stride + 4 * x + 1] = (g * a) / 255; >- targetData[y * stride + 4 * x + 2] = (r * a) / 255; >- targetData[y * stride + 4 * x + 3] = a; >+ res.targetData[y * res.stride + 4 * x] = (b * a) / 255; >+ res.targetData[y * res.stride + 4 * x + 1] = (g * a) / 255; >+ res.targetData[y * res.stride + 4 * x + 2] = (r * a) / 255; >+ res.targetData[y * res.stride + 4 * x + 3] = a; > } > >- fixupTarget(sourceData, targetData, width, height, stride, rect); >- >- sourceImage->Unlock(); >- targetImage->Unlock(); >- >- instance->DefineImage(result, targetImage); >- instance->DefineRegion(result, rect); >- >+ ReleaseResources(instance, &res); > return NS_OK; > } > > NS_IMETHODIMP > nsSVGFEComponentTransferElement::GetRequirements(PRUint32 *aRequirements) > { > *aRequirements = CheckStandardNames(mIn1); > return NS_OK; >@@ -1395,19 +1383,16 @@ nsSVGFEMergeElement::~nsSVGFEMergeElemen > // nsIDOMNode methods > > NS_IMPL_DOM_CLONENODE_WITH_INIT(nsSVGFEMergeElement) > > NS_IMETHODIMP > nsSVGFEMergeElement::Filter(nsSVGFilterInstance *instance) > { > nsRect rect; >- nsIDOMSVGFilterPrimitiveStandardAttributes *attrib = >- (nsIDOMSVGFilterPrimitiveStandardAttributes *)this; >- > nsCOMPtr<nsISVGRendererSurface> sourceImage, targetImage; > instance->GetImage(getter_AddRefs(targetImage)); > if (!targetImage) > return NS_ERROR_FAILURE; > > PRUint8 *sourceData, *targetData; > PRInt32 stride; > PRUint32 width, height, length; >@@ -1457,17 +1442,17 @@ nsSVGFEMergeElement::Filter(nsSVGFilterI > BLEND(targetData[y * stride + 4 * x + 1], > sourceData[y * stride + 4 * x + 1], a); > BLEND(targetData[y * stride + 4 * x + 2], > sourceData[y * stride + 4 * x + 2], a); > BLEND(targetData[y * stride + 4 * x + 3], > sourceData[y * stride + 4 * x + 3], a); > } > #undef BLEND >- >+ > sourceImage->Unlock(); > } > > fixupTarget(nsnull, targetData, width, height, stride, rect); > > targetImage->Unlock(); > > nsAutoString result; >@@ -1804,89 +1789,57 @@ NS_IMETHODIMP nsSVGFEOffsetElement::GetD > *aDy = mDy; > NS_IF_ADDREF(*aDy); > return NS_OK; > } > > NS_IMETHODIMP > nsSVGFEOffsetElement::Filter(nsSVGFilterInstance *instance) > { >- nsRect rect; >- nsIDOMSVGFilterPrimitiveStandardAttributes *attrib = >- (nsIDOMSVGFilterPrimitiveStandardAttributes *)this; >- >- nsRect defaultRect; >- nsAutoString input, result; >- mIn1->GetAnimVal(input); >- mResult->GetAnimVal(result); >- instance->LookupRegion(input, &defaultRect); >- >- instance->GetFilterSubregion(this, defaultRect, &rect); >+ nsSVGFilterResource res; >+ nsresult rv = ObtainResources(instance, mIn1, &res); >+ NS_ENSURE_SUCCESS(rv,rv); nit: space after comma. > > #ifdef DEBUG_tor > fprintf(stderr, "FILTER OFFSET rect: %d,%d %dx%d\n", >- rect.x, rect.y, rect.width, rect.height); >+ res.rect.x, res.rect.y, >+ res.rect.width, res.rect.height); > #endif > >- nsCOMPtr<nsISVGRendererSurface> sourceImage, targetImage; >- instance->LookupImage(input, getter_AddRefs(sourceImage)); >- if (!sourceImage) >- return NS_ERROR_FAILURE; >- >- instance->GetImage(getter_AddRefs(targetImage)); >- if (!targetImage) >- return NS_ERROR_FAILURE; >- >- PRUint8 *sourceData, *targetData; >- PRInt32 stride; >- PRUint32 width, height, length; >- sourceImage->Lock(); >- targetImage->Lock(); >- sourceImage->GetData(&sourceData, &length, &stride); >- targetImage->GetData(&targetData, &length, &stride); >- >- sourceImage->GetWidth(&width); >- sourceImage->GetHeight(&height); >- > PRInt32 offsetX, offsetY; > float flt; > nsSVGLength2 val; > > mDx->GetAnimVal(&flt); > val.Init(nsSVGUtils::X, 0xff, flt, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); > offsetX = (PRInt32) instance->GetPrimitiveLength(&val); > > mDy->GetAnimVal(&flt); > val.Init(nsSVGUtils::Y, 0xff, flt, nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER); > offsetY = (PRInt32) instance->GetPrimitiveLength(&val); > >- for (PRInt32 y = rect.y; y < rect.y + rect.height; y++) { >+ for (PRInt32 y = res.rect.y; >+ y < res.rect.y + res.rect.height; y++) { > PRInt32 targetRow = y + offsetY; >- if (targetRow < rect.y || targetRow >= rect.y + rect.height) >+ if (targetRow < res.rect.y || >+ targetRow >= res.rect.y + res.rect.height) > continue; If you had a getRect member function on nsSVGFilterResource and kept the local nsRect rect then the above code could be unchanged. > >- PRInt32 targetColumn = rect.x + offsetX; >- if (targetColumn < rect.x) >- memcpy(targetData + stride * targetRow + 4 * rect.x, >- sourceData + stride * y - 4 * offsetX, >- 4 * (rect.width + offsetX)); >+ PRInt32 targetColumn = res.rect.x + offsetX; >+ if (targetColumn < res.rect.x) >+ memcpy(res.targetData + res.stride * targetRow + 4 * res.rect.x, >+ res.sourceData + res.stride * y - 4 * offsetX, >+ 4 * (res.rect.width + offsetX)); cf. above with getTargetData. > else >- memcpy(targetData + stride * targetRow + 4 * targetColumn, >- sourceData + stride * y + 4 * rect.x, >- 4 * (rect.width - offsetX)); >+ memcpy(res.targetData + res.stride * targetRow + 4 * targetColumn, >+ res.sourceData + res.stride * y + 4 * res.rect.x, >+ 4 * (res.rect.width - offsetX)); > } > >- fixupTarget(sourceData, targetData, width, height, stride, rect); >- >- sourceImage->Unlock(); >- targetImage->Unlock(); >- >- instance->DefineImage(result, targetImage); >- instance->DefineRegion(result, rect); >- >+ ReleaseResources(instance, &res); > return NS_OK; > } > > NS_IMETHODIMP > nsSVGFEOffsetElement::GetRequirements(PRUint32 *aRequirements) > { > *aRequirements = CheckStandardNames(mIn1); > return NS_OK; >Index: content/svg/content/src/nsSVGFilters.h >=================================================================== >RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGFilters.h,v >retrieving revision 1.1 >diff -u -8 -p -r1.1 nsSVGFilters.h >--- content/svg/content/src/nsSVGFilters.h 14 Apr 2006 15:09:38 -0000 1.1 >+++ content/svg/content/src/nsSVGFilters.h 14 Jun 2006 21:22:07 -0000 >@@ -35,33 +35,47 @@ > * ***** END LICENSE BLOCK ***** */ > > #ifndef __NS_SVGFILTERSELEMENT_H__ > #define __NS_SVGFILTERSELEMENT_H__ > > #include "nsSVGStylableElement.h" > #include "nsSVGLength2.h" > >+typedef struct{ >+ PRUint8 *sourceData, *targetData; >+ PRUint32 width, height, length; >+ PRInt32 stride; >+ nsRect rect; >+ nsAutoString input, result; >+ nsCOMPtr<nsISVGRendererSurface> sourceImage, targetImage; >+}nsSVGFilterResource; >+ Better as a class with member functions as alluded to above. Member variables should be mSourceData, mTargetData etc. > typedef nsSVGStylableElement nsSVGFEBase; > > class nsSVGFE : public nsSVGFEBase > //, public nsIDOMSVGFilterPrimitiveStandardAttributes > { > friend class nsSVGFilterInstance; > > protected: > nsSVGFE(nsINodeInfo *aNodeInfo); > nsresult Init(); > > // nsISVGValueObserver interface: > NS_IMETHOD WillModifySVGObservable(nsISVGValue* observable, > nsISVGValue::modificationType aModType); >- NS_IMETHOD DidModifySVGObservable (nsISVGValue* observable, >- nsISVGValue::modificationType aModType); >- >+ NS_IMETHOD DidModifySVGObservable(nsISVGValue* observable, >+ nsISVGValue::modificationType aModType); Not sure what's changed in the above lines. >+ NS_IMETHODIMP ObtainResources(nsSVGFilterInstance* aInstance, NS_METHODIMP is for the .cpp implementation. Having said that I think this should return nsresult. >+ nsCOMPtr<nsIDOMSVGAnimatedString> aIn1, >+ nsSVGFilterResource* aResource); >+ void ReleaseResources(nsSVGFilterInstance* aInstance, >+ nsSVGFilterResource* aResource); >+ > public: > // interfaces: > NS_DECL_ISUPPORTS_INHERITED > NS_DECL_NSIDOMSVGFILTERPRIMITIVESTANDARDATTRIBUTES > > // nsSVGElement specializations: > virtual void DidChangeLength(PRUint8 aAttrEnum, PRBool aDoSetAttr); >
Attachment #225621 - Flags: review?(longsonr) → review-
Oops, should have edited that down a bit.
Attached patch Updated Patch (obsolete) — — Splinter Review
A more object oriented approach to code consolidation
Attachment #225621 - Attachment is obsolete: true
Attachment #225904 - Flags: superreview?
Attachment #225904 - Flags: review?(jwatt)
Attachment #225621 - Flags: superreview?(tor)
Comment on attachment 225904 [details] [diff] [review] Updated Patch I thought I'd continue with my 2c since I started. >+//--------------------Filter Resource----------------------- >+ >+ >+nsSVGFR::~nsSVGFR() >+{ >+ ReleaseTarget(); >+ ReleaseSource(); >+} >+ >+ >+} >+ >+void >+nsSVGFR::ReleaseTarget() >+ mTargetImage=nsnull; nit: spaces round =. And in various other places. > NS_IMETHODIMP > nsSVGFEGaussianBlurElement::Filter(nsSVGFilterInstance *instance) > { >+ nsSVGFR fr(instance); >+ code elided... >- >- instance->DefineImage(result, targetImage); >- instance->DefineRegion(result, rect); > >+ fr.ReleaseTarget(); >+ fr.ReleaseSource(); Aren't you calling ReleaseTarget and ReleaseSource twice, i.e. also in the destructor? This occurs in various other methods too. You set the variables to null in the release methods so the subsequent releases do nothing but it seems unecessary.
Attached patch Updated Patch (obsolete) — — Splinter Review
Added spaces around "=" and removed redundant Release() calls.
Attachment #225904 - Attachment is obsolete: true
Attachment #225904 - Flags: superreview?
Attachment #225904 - Flags: review?(jwatt)
Attachment #226170 - Flags: superreview?(tor)
Attachment #226170 - Flags: review?(jwatt)
Comment on attachment 226170 [details] [diff] [review] Updated Patch >? .cdtproject >? .project >? content/svg/content/src/.nsSVGFilters.cpp.marks >? security/nss/cmd/shlibsign/NONE >? security/nss/cmd/shlibsign/mangle/NONE >? security/nss/lib/ckfw/builtins/NONE >? security/nss/lib/freebl/NONE >? security/nss/lib/nss/NONE >? security/nss/lib/smime/NONE >? security/nss/lib/softoken/NONE >? security/nss/lib/ssl/NONE >Index: content/svg/content/src/nsSVGFilters.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/svg/content/src/nsSVGFilters.cpp,v >retrieving revision 1.5 >diff -u -8 -p -r1.5 nsSVGFilters.cpp >--- content/svg/content/src/nsSVGFilters.cpp 14 Apr 2006 15:09:38 -0000 1.5 >+++ content/svg/content/src/nsSVGFilters.cpp 19 Jun 2006 16:27:39 -0000 >@@ -173,16 +173,171 @@ nsSVGFE::DidChangeLength(PRUint8 aAttrEn > > nsSVGElement::LengthAttributesInfo > nsSVGFE::GetLengthInfo() > { > return LengthAttributesInfo(mLengthAttributes, sLengthInfo, > NS_ARRAY_LENGTH(sLengthInfo)); > } > >+//--------------------Filter Resource----------------------- >+ >+class nsSVGFR Rename this to something that will be more obvious for the next person who has to maintain the code, like nsSVGFilterResource. >+{ >+protected: >+ void FixupTarget(); >+ >+public: >+ nsSVGFR(nsSVGFilterInstance* aInstance); >+ ~nsSVGFR(); >+ >+ nsresult AcquireSourceImage(nsCOMPtr<nsIDOMSVGAnimatedString> aIn1, >+ nsCOMPtr<nsIDOMSVGAnimatedString> aResult, >+ nsSVGFE* aFilter, PRUint8** aSourceData); Don't accept a "nsCOMPtr<foo>" as an argument, but rather a "foo*". >+nsresult >+nsSVGFR::AcquireTargetImage(PRUint8** aTargetData) >+{ >+ mInstance->GetImage(getter_AddRefs(mTargetImage)); >+ if (!mTargetImage) >+ return NS_ERROR_FAILURE; "if (...)" >+void >+nsSVGFR::ReleaseSource() >+{ >+ if(!mSourceImage) >+ return; "if (...)" >+void >+nsSVGFR::ReleaseTarget() >+{ >+ if(!mTargetImage) >+ return; "if (...)" > NS_IMETHODIMP > nsSVGFEMergeElement::Filter(nsSVGFilterInstance *instance) > { ... >+ rv = fr.AcquireSourceImage(str, mResult, this, &sourceData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ fr.GetRect(&rect); > > #ifdef DEBUG_tor > fprintf(stderr, "FILTER MERGE rect: %d,%d %dx%d\n", > rect.x, rect.y, rect.width, rect.height); > #endif > >- instance->LookupImage(input, getter_AddRefs(sourceImage)); >- if (!sourceImage) { >- targetImage->Unlock(); >- return NS_ERROR_FAILURE; >+ if (NS_FAILED(rv)) { >+ fr.ReleaseTarget(); >+ return rv; The NS_ENSURE_SUCCESS after AquireSourceImage is already doing the early return (and the destructor picking up the ReleaseTarget). Remove this check.
Attachment #226170 - Flags: superreview?(tor)
Attachment #226170 - Flags: review?(jwatt)
Attachment #226170 - Flags: review-
Attached patch Updated Patch (obsolete) — — Splinter Review
Fixed class name, nsCOMPtr arguments, and “if” spacing
Attachment #226170 - Attachment is obsolete: true
Attachment #226711 - Flags: superreview?
Attachment #226711 - Flags: review?(jwatt)
Attachment #226711 - Flags: superreview? → superreview?(tor)
Comment on attachment 226711 [details] [diff] [review] Updated Patch The first comment I have here is that it is *very* helpful to comment code as you go - both the code you add, and existing code that is somehow involved in the changes you're making. This code is badly underdocumented and adding comments would help speed up review as well as ease future maintenance. Whenever you work on some code, you're going to have questions about it. Please consider adding documentation as you figure out the answers. I'd really love to see that being part of the process everyone goes through as they work on a patch. For example, what does FixupTarget actually do? What is GetFilterSubregion actually getting (what is a filter subregion)? For those that don't know the answers, there should be short comments before the declarations of functions and class members so they don't need to analyse the functions and the functions they call, etc. and they don't need to look to see how members are used.
Attached patch Patch w/ comments (obsolete) — — Splinter Review
This updated to the patch adds documentation to nsSVGFilterResource and modifies the arguments and visibility of some of nsSVGFilterResource's methods in order to simplify usage.
Attachment #226711 - Attachment is obsolete: true
Attachment #226711 - Flags: superreview?(tor)
Attachment #226711 - Flags: review?(jwatt)
Thanks Alex, I really appreciate you going to the trouble to add the comments!!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to comment #13) > Thanks Alex, I really appreciate you going to the trouble to add the comments!! > No problem! Everybody loves comments. I hope there are enough in there to clear things up, if not just let me know what requires a more thorough explanation.
Comment on attachment 231447 [details] [diff] [review] Patch w/ comments Here are some initial comments. My first comment is that in the Mozilla source comments documenting methods and members go where the method/member is declared, not where they are defined. That's where moz contributors expect to find comments, so I'd be grateful if you could move them accordingly. For example, see: http://lxr.mozilla.org/seamonkey/source/layout/base/nsFrameManager.h >+//--------------------Filter Resource----------------------- >+/* nsSVGFilterResource provides functionality for obtaining and releasing >+ * target and source images. Images are automatically released when the Nit: maybe "source and target" (reverse order)? >+ * Filter Resource is destroyed, but methods to manually release images are >+ * provided so that multiple source images may be used for a single filter. Hmm. Maybe ReleaseImage should be protected and called automatically at the start of AcquireSourceImage? That eliminates the possibility of AcquireSourceImage being called twice at some point in the future without calling ReleaseImage between those calls. >+ */ >+class nsSVGFilterResource >+{ >+protected: >+ void FixupTarget(); >+ void ReleaseTarget(); Please put this down at the top of the next protected section before the members (followed by a blank line). Best to have all the public stuff first IMO. It's also best to have the definitions and declarations in the same order in the file (i.e. please move the definition of FixupTarget down to after the definition of GetDataDimensions). >+public: >+ nsSVGFilterResource(); >+ nsSVGFilterResource(nsSVGFilterInstance* aInstance); >+ ~nsSVGFilterResource(); >+ >+ nsresult AcquireSourceImage(nsIDOMSVGAnimatedString* aIn1, >+ nsSVGFE* aFilter, PRUint8** aSourceData); >+ nsresult AcquireTargetImage(nsIDOMSVGAnimatedString* aResult, >+ PRUint8** aTargetData); >+ void ReleaseSource(); >+ void GetRect(nsRect* aRect); >+ PRUint32 GetDataLength(); >+ PRInt32 GetDataStride(); >+ >+protected: >+ nsSVGFilterInstance* mInstance; >+ PRUint8 *mSourceData, *mTargetData; >+ PRUint32 mWidth, mHeight, mLength; >+ PRInt32 mStride; >+ nsRect mRect; >+ nsAutoString mInput, mResult; nsAutoString is intended for use on the stack, not the heap, so members should be of type nsString instead. See: http://developer.mozilla.org/en/docs/XPCOM:Strings#Appendix_A_-_What_class_to_use_when I realise that nsSVGFilterResource is only ever used on the stack itself, so I guess it actually makes sense to use nsAutoString though. Maybe leave it as it is and add a short comment something like: // nsSVGFilterResource is only used on the stack, hence nsAutoString members >+ nsCOMPtr<nsISVGRendererSurface> mSourceImage, mTargetImage; >+}; >+ >+/* Default constructor provided to conform to >+ * portability guidelines only, not intended for use. */ >+nsSVGFilterResource::nsSVGFilterResource() >+{ >+} The portability guidelines are very out of date in places. There are so many classes that don't have default constructors that I think it's safe to say this is one of them. As far as I'm concerned this should be removed to avoid unnecessary clutter and distraction. >+/* Acquiring a source image will lock that image >+ * and prepare its data to be read. >+ * ain1: is a pointer to the name of image to load Capital "I" in aIn1. Also, I wonder why we need the "1" since it isn't handed multiple inputs at the same time. Some (hopefully) constructive critisism regarding comments: only use comments to tell me things that aren't obvious and that I may not know. For example, looking down I can easily see that aIn1 is of type nsIDOMSVGAnimatedString*, therefore the "is a pointer" bit is somewhat redundant. Good comments tell you just enought, but not too much, and they don't exist just for the sake of it. Otherwise they waste space and the time of whoever reads them. Not that I'm great at writing good comments myself, but those are a couple of things I'd note. :-) In this case I'd rename the argument to "aIn" and change the comment to something like "the name of the filter primitive to use as the source". >+ * aFilter: is a pointer to the filter that is calling AcquireImage "the filter that is calling Acquire*Source*Image"? >+ * aSourceData: is a return parameter that will point to the source >+ * image data by method completion. I think that should be _out_ parameter, not "return" paremeter. Also I'd mention "filter primitive" again in there. Perhaps "out parameter - the image data of the filter primitive specified by aIn" >+ */ >+nsresult >+nsSVGFilterResource::AcquireSourceImage(nsIDOMSVGAnimatedString* aIn1, >+ nsSVGFE* aFilter, PRUint8** aSourceData) >+{ >+ nsRect defaultRect; >+ aIn1->GetAnimVal(mInput); >+ mInstance->LookupRegion(mInput, &defaultRect); >+ >+ mInstance->GetFilterSubregion(aFilter, defaultRect, &mRect); >+ mInstance->LookupImage(mInput, >+ getter_AddRefs(mSourceImage)); Just put that on one line, it doesn't exceed 80 chars. >+ if (!mSourceImage) >+ return NS_ERROR_FAILURE; Content style should be to wrap even one line in braces, at least for new code and code that's being touched anyway. This comment applies in several other places in this patch too. >+/* Acquiring a target image will create and lock >+ * a new surface to be used as the target image. >+ * aResult: is a pointer to a name that will be assigned to the target >+ * image when it is released. Perhaps "the name by which the resulting filter primitive image will be identified"? >+ * aTargetData: is a return parameter that will be set to point to the target >+ image data by method completion. "out parameter - the resulting filter primitive image data"? >+ * Iff a source image exits, it is unlocked >+ */ .... >+/* Iff a target image exists, >+ * FixupTarget is called and the image is unlocked. >+ */ Iff? :-) Also I'm wondering why FixupTarget has to be called. And that comment is broken at a funny place. >+/* >+ * FixupTarget copies the parts of the source image that are outside of the >+ * filter subregion into the target image. If no source image exists, then the >+ * area outside the filter region is filled with black and opacity 0. >+ */ Great comment. Very helpful. :-) I would change the "are outside" to "lie outside", and "black and opacity 0" to "transparent black" though. >+void >+nsSVGFilterResource::GetRect(nsRect* aRect) >+{ >+ *aRect = mRect; >+} Why use an out param? Why not just have this method return the nsRect*?
Attached patch Patch w/ better comments (obsolete) — — Splinter Review
Thanks for all the great comments! FixupTarget is called in ReleaseTarget in order to copy the unfiltered portion of the source graphic into the target image. This occurs when a subfilter specifies a region smaller than its parents. The specification is a little fuzzy as to whether or not data outside the filter region should be copied, but I am going along with tor's interpretation http://lists.w3.org/Archives/Public/www-svg/2006Jan/0432 Also, please note that I have moved the initialization of mWidth and mHeight from AcquireSourceImage() into AcquireTargetImage(). I believe these is necessary to make FixupTarget will work correctly with filters that do not use a source image (such as feTurbulence) when they are implemented. To the best of my knowledge, all images are the same size of their filter region. Thus, it shouldn't make any difference whether the sourceImage or the targetImage is used to initialize mWidth and mHeight.
Attachment #231447 - Attachment is obsolete: true
Attachment #231684 - Flags: review?(jwatt)
Attachment #231447 - Flags: review?(jwatt)
Comment on attachment 231684 [details] [diff] [review] Patch w/ better comments A couple minor nits to fix: * order nsSVGFilterResource members by size for better packing (nsAutoString, nsRect, pointers, the rest). * we're not inheriting from nsSVGFilterResource, so the protected portion could be private instead.
Attachment #231684 - Flags: review?(jwatt) → review+
Attached patch nit fix (obsolete) — — Splinter Review
Patch with nits fixed and trailing whitespace removed.
Attachment #231684 - Attachment is obsolete: true
Attachment #238221 - Flags: superreview?(roc)
Comment on attachment 238221 [details] [diff] [review] nit fix +nsRect +nsSVGFilterResource::GetRect() +{ + return mRect; +} + +PRUint32 +nsSVGFilterResource::GetDataLength() +{ + return mLength; +} + +PRInt32 +nsSVGFilterResource::GetDataStride() +{ + return mStride; +} + Make these inline. Make GetRect() return a const nsRect&. How do we know that the source and target images have the same mLength and mStride? (I know we already depended on that, I'm just curious.)
Attachment #238221 - Flags: superreview?(roc) → superreview+
All images for a given filter are created to be the same size as the filter region. Thus, the source and target images for a particular filter must be the same size and have the same mLength and mStride.
Attachment #238221 - Attachment is obsolete: true
Checked in for Alex.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: