Bug 49907

Summary: Better result passing in filter primitives
Product: WebKit Reporter: Zoltan Herczeg <zherczeg>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: krit, mdelaney7, oliver, simon.fraser, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
concept
none
updated concept
none
patch
krit: review-
patch 2
zimmermann: review-
patch 3
krit: review-
patch 4 krit: review+

Description Zoltan Herczeg 2010-11-22 06:50:08 PST
Filter primitives always converts their result to image buffer, and getting the image from it. This is really expensive operation on some ports, especially on Qt. I have started to work on a better result passing behaviour. The patch is highly conceptual, not finished, etc., but you can see what I plan to do. What is your opinion about this approach?
Comment 1 Zoltan Herczeg 2010-11-22 06:50:35 PST
Created attachment 74548 [details]
concept
Comment 2 Dirk Schulze 2010-11-22 07:23:39 PST
In general I think this is a great idea! But I couldn't look into detail. Reading your patch tis evening. But the idea sounds great.
Comment 3 Zoltan Herczeg 2010-11-23 06:42:53 PST
Created attachment 74657 [details]
updated concept

Working on small tests (hopefully mostly finished). Would like to hear your opinion.
Comment 4 Dirk Schulze 2010-11-23 10:01:48 PST
Comment on attachment 74657 [details]
updated concept

View in context: https://bugs.webkit.org/attachment.cgi?id=74657&action=review

Even if I really like the idea, I fear that we and up with 3 image results per FilterEffect. But of course I see the performance win. But we also have to think about hardware acceleration. Some platforms support effects directly and I always saw the software rendering as a fallback if a platform does not support one or more effects. Eg on a Mac we can emulate all effects, on Skia and OpenVG we can at least emulate many effects, while we don't have effects on Qt and Cairo _yet_! At the moment every FilterEffect just has the ImageBuffer and the absolutePaintRect, so that it should be possible to replace some effects by platform effects and use the fallback on other ones. If we use your design, it will be much harder and maybe impossible to use platform effects. And I bet even Cairo and Qt will implement effects sooner or later. Do you have some proposals?

> WebCore/platform/graphics/filters/FEComposite.cpp:150
> +    GraphicsContext* filterContext = resultImage->context();

return if a filterContext does not exist. Make sure thet this->hasResult does not return true.

> WebCore/platform/graphics/filters/FEConvolveMatrix.cpp:396
> +    RefPtr<ImageData> imageData = ImageData::create(absolutePaintRect().width(), absolutePaintRect().height());

Save absolutePaintRect().size() locally.

> WebCore/platform/graphics/filters/FEDisplacementMap.cpp:112
> +    int stride = absolutePaintRect().width() * 4;

is it better to save absolutePaintRect() in a local variable here?

> WebCore/platform/graphics/filters/FEFlood.cpp:72
> +    resultImage->context()->fillRect(FloatRect(FloatPoint(), absolutePaintRect().size()), color, ColorSpaceDeviceRGB);

Make sure that getting the context was successful.

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:209
> +            boxBlur(tmpPixelArray, srcPixelArray, kernelSizeY, dyLeft, dyRight, stride, 4, absolutePaintRect().height(), absolutePaintRect().width(), isAlphaImage());

Save absolutePaintRect() in a local variable

> WebCore/platform/graphics/filters/FEOffset.cpp:91
> +    resultImage->context()->drawImageBuffer(in->asImageBuffer(), ColorSpaceDeviceRGB, drawingRegion);

check for the context first.

> WebCore/platform/graphics/filters/FETile.cpp:83
> +    resultImage->context()->setFillPattern(pattern);

Ditto.

> WebCore/platform/graphics/filters/FETurbulence.cpp:327
> +    if (!absolutePaintRect().width() || !absolutePaintRect().height())

save the size of absolutePaintRect in a local variable

> WebCore/platform/graphics/filters/FilterEffect.cpp:104
> +inline void FilterEffect::copyImageBytes(ImageData* dst, ImageData* src, const IntRect& rect)

just a minor detail, can you switch src an dst, please? :-)

> WebCore/platform/graphics/filters/FilterEffect.cpp:107
> +    ASSERT(dst->width() == rect.width() && dst->height() == rect.height());

don't we have an operator== for IntSize? I'd prefer IntSize(dst->width(), dst->height()) == rect.size() Why don't we have a IntSize for ImageData? Could you take a look at it?

> WebCore/platform/graphics/filters/FilterEffect.cpp:172
> +            m_unmultipliedImageResult = m_imageBufferResult->getUnmultipliedImageData(IntRect(IntPoint(), m_absolutePaintRect.size()));

I guess it really depends on the platform if it is faster to do it manually here. Some platforms just copy the data. Can you take a look at this again please?

> WebCore/platform/graphics/filters/FilterEffect.cpp:198
> +            m_premultipliedImageResult = m_imageBufferResult->getPremultipliedImageData(IntRect(IntPoint(), m_absolutePaintRect.size()));

Ditto.

> WebCore/platform/graphics/filters/SourceGraphic.cpp:59
> +    resultImage->context()->drawImageBuffer(filter->sourceImage(), ColorSpaceDeviceRGB, IntPoint());

I'm really unsure if it can happen that a GC doesn't exist while the ImageBuffer does. Is it absolutely save? Can check this please? You're using this pattern on quite some other places.
Comment 5 Zoltan Herczeg 2010-11-23 10:31:22 PST
> Even if I really like the idea, I fear that we and up with 3 image results per FilterEffect. But of course I see the performance win. But we also have to think about hardware acceleration. 

I have no strong opinion here. I know most filters (light, turbulence, gaussian blur, morphology, convolve matrix, displacement) will never be hw accelerated except the simplest ones (offset, merge). And yeah, we can have 3 results for each filters, although the filter itself only creates one, and the others are created on demand. But you should decide whether it is worth to work on this project. That is why I submit conceptual patches, since if this is not a good idea, at least I don't have to spend full days working on it.
Comment 6 Dirk Schulze 2010-11-23 10:48:25 PST
(In reply to comment #5)
> > Even if I really like the idea, I fear that we and up with 3 image results per FilterEffect. But of course I see the performance win. But we also have to think about hardware acceleration. 
> 
> I have no strong opinion here. I know most filters (light, turbulence, gaussian blur, morphology, convolve matrix, displacement) will never be hw accelerated except the simplest ones (offset, merge). And yeah, we can have 3 results for each filters, although the filter itself only creates one, and the others are created on demand. But you should decide whether it is worth to work on this project. That is why I submit conceptual patches, since if this is not a good idea, at least I don't have to spend full days working on it.

Yes, thanks for the great proposal, but maybe others should also comment on this bug.

To the hwa. I know of OpenVG that they support fast filter effects for gaussian blur and convolve matrix, not sure how they calculate it. But I wouldn't say for sure that they can't be faster with hwa :-)
And I'm pretty sure that these filter can be a lot faster on a GPU because they are very good to parallelize.

Adding Simon and Oliver, since hwa is not my field of knowledge.
Comment 7 Zoltan Herczeg 2010-11-24 00:31:31 PST
If we ever want to support hw accelerators, they would require a special code path (specialized apply functions), and we need to write specialized code for them. Thus, this could be a sw rendering (SMP accelerated) code path, which could be enabled by compiler directives. The 3 new functions can fallback to the original ImageBuffer based implementations when we don't want to use these buffers in a mixed (sh/hw accelerated) environment.
Comment 8 Dirk Schulze 2010-11-24 05:09:24 PST
(In reply to comment #7)
> If we ever want to support hw accelerators, they would require a special code path (specialized apply functions), and we need to write specialized code for them. Thus, this could be a sw rendering (SMP accelerated) code path, which could be enabled by compiler directives. The 3 new functions can fallback to the original ImageBuffer based implementations when we don't want to use these buffers in a mixed (sh/hw accelerated) environment.

Talked about this topic with zherzeg on IRC. It should still be possible to combine HW and SW rendering if needed. The ImageBuffer is always accessible like it is right now.
Comment 9 Zoltan Herczeg 2010-11-24 05:13:43 PST
> > WebCore/platform/graphics/filters/FEComposite.cpp:150
> > +    GraphicsContext* filterContext = resultImage->context();
> 
> return if a filterContext does not exist. Make sure thet this->hasResult does not return true.

Thinking about this. Is it possible, that the image is allocated, but the context is not? Anyway, I would add this check to FilterEffect::createImageBufferResult() (All filters, which allocates an imageBuffer will use the context!) An ASSERT(!hasResult()) added to all create.*Result functions.
Comment 10 Zoltan Herczeg 2010-11-25 03:45:28 PST
Created attachment 74848 [details]
patch

This patch is tested and working (On mac leopard with -p and --tolerance 0).
Comment 11 Dirk Schulze 2010-11-26 13:20:41 PST
Comment on attachment 74848 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74848&action=review

The style bot is red, did you check the style manually on your machine?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:179
> +    in->asPremultipliedImage(resultImage, effectDrawingRect);

Can you explain how this works? the resultImage is possibly larger than the sourceImage, in every direction. How do you copy the data to the resultImage?

> WebCore/platform/graphics/filters/FEGaussianBlur.cpp:192
> +    RefPtr<ImageData> tmpImageData = ImageData::create(absolutePaintRect().width(), absolutePaintRect().height());
>      ByteArray* tmpPixelArray = tmpImageData->data()->data();
>  
> -    int stride = 4 * imageRect.width();
> +    IntSize paintSize = absolutePaintRect().size();

Can you move the intsize some line above and replace the absolutePainRect() calls?

> WebCore/platform/graphics/filters/FETile.cpp:84
> +    resultImage->context()->setFillPattern(pattern);
> +    resultImage->context()->fillRect(FloatRect(FloatPoint(), absolutePaintRect().size()));

save context in a local var before using it here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:78
> +    // This function is allowed to return with NULL.

I think this sentence is unnecessary.

> WebCore/platform/graphics/filters/FilterEffect.cpp:94
> +    PassRefPtr<ImageData> imageData = ImageData::create(rect.width(), rect.height());

You have to take the ownership first: RefPtr<ImageData>

> WebCore/platform/graphics/filters/FilterEffect.cpp:101
> +    PassRefPtr<ImageData> imageData = ImageData::create(rect.width(), rect.height());

Ditto.

> WebCore/platform/graphics/filters/FilterEffect.cpp:223
> +    if (!m_imageBufferResult->context()) {
> +        // All filter effects, which allocates an imageBufferResult
> +        // will use its context. If we would not destroy the image here
> +        // the filter would return a black rectangle, which is not desired.
> +        // (I would prefer an ASSERT here, since I think the context must be exists)
> +        m_imageBufferResult.clear();
> +    }

I agree. ;)
Comment 12 Zoltan Herczeg 2010-11-29 00:56:21 PST
Created attachment 74995 [details]
patch 2

Fixed everything.
Comment 13 Zoltan Herczeg 2010-11-29 01:02:21 PST
> Can you explain how this works? the resultImage is possibly larger than the sourceImage, in every direction. How do you copy the data to the resultImage?

Same way as get/put pre/unmultiplied data-s. If the rect is bigger than the source area, it fills with 0 first (transparent black pixels).
Comment 14 Zoltan Herczeg 2010-11-29 01:24:02 PST
> The style bot is red, did you check the style manually on your machine?

No idea about this. It did not report an error, and now it is green... Perhaps an internal error?
Comment 15 Nikolas Zimmermann 2010-11-29 12:14:56 PST
Comment on attachment 74995 [details]
patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=74995&action=review

I love the general idea of this patch! Some comments below, Dirk might have additional ones:

> WebCore/ChangeLog:8
> +        SVN filter primitives can use the output of other filters. The

s/SVN/SVG/

> WebCore/ChangeLog:11
> +        primitive result was converted to image buffer before, which

s/result was/results were/, s/buffer/buffers/

> WebCore/ChangeLog:15
> +        this patch. All apply() methods updated according to this new

s/updated/are updated/

> WebCore/platform/graphics/filters/FEBlend.cpp:-135
> -    resultImage()->putPremultipliedImageData(imageData.get(), imageRect, IntPoint());

Ah, this is correct, as you're using the resultImage instead of creating a new one.

> WebCore/platform/graphics/filters/FEComponentTransfer.cpp:172
> +    in->asUnmultipliedImage(imageData, drawingRect);

Hm, why is the return value ignored?
(Ignore this, I was confused as there are two variants of asUnmultipliedImage, this one returns void.... see below)

> WebCore/platform/graphics/filters/FilterEffect.cpp:84
> +    m_imageBufferResult = ImageBuffer::create(m_absolutePaintRect.size(), ColorSpaceLinearRGB);
> +    IntRect dst(IntPoint(), m_absolutePaintRect.size());

line 84: s/dst/destinationRect/

> WebCore/platform/graphics/filters/FilterEffect.cpp:96
> +    return imageData;

A call to .release() is missing here!

> WebCore/platform/graphics/filters/FilterEffect.cpp:103
> +    return imageData;

Ditto.

> WebCore/platform/graphics/filters/FilterEffect.cpp:106
> +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst, const IntRect& rect)

Avoid abbrevations, while it is convienent here, I'd prefer source and destination.

> WebCore/platform/graphics/filters/FilterEffect.cpp:113
> +    int originx = rect.x();

I'd prefer originX or xOrigin here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:114
> +    int destx = 0;

Same here, destX or xDest (I'd prefer the latter ;-)

> WebCore/platform/graphics/filters/FilterEffect.cpp:119
> +    int endx = rect.x() + rect.width();

rect.right() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:129
> +    int endy = rect.y() + rect.height();

rect.bottom() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:137
> +    int dstScanline = rect.width() * 4;
> +    int srcScanline = m_absolutePaintRect.width() * 4;
> +    unsigned char *dstPixel = dst->data()->data()->data() + (((desty * rect.width()) + destx) * 4);
> +    unsigned char *srcPixel = src->data()->data()->data() + (((originy * m_absolutePaintRect.width()) + originx) * 4);

unsigned char* destinationPixel,  and one useless pair of braces at the end. Same for sourcePixel.
dstScanline should be "destinationScanline", same for srcScanline.

> WebCore/platform/graphics/filters/FilterEffect.cpp:143
> +        originy++;

I'd prefer ++originy.

> WebCore/platform/graphics/filters/FilterEffect.cpp:151
> +    if (!m_unmultipliedImageResult) {

Can you rewrite this with a early return for m_unmultipliedImageResult=true.

> WebCore/platform/graphics/filters/FilterEffect.cpp:159
> +            unsigned char* src = m_premultipliedImageResult->data()->data()->data();
> +            unsigned char* dst = m_unmultipliedImageResult->data()->data()->data();
> +            unsigned char* end = src + (m_absolutePaintRect.width() * m_absolutePaintRect.height() * 4);

Again, src -> source, dst -> destination etc.

> WebCore/platform/graphics/filters/FilterEffect.cpp:184
> +    if (!m_premultipliedImageResult) {

Same comments as abobe, early return style please, and variable renamings.

> WebCore/platform/graphics/filters/FilterEffect.h:58
> +    PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&);
> +    PassRefPtr<ImageData> asPremultipliedImage(const IntRect&);
> +    void asUnmultipliedImage(ImageData* dst, const IntRect&);
> +    void asPremultipliedImage(ImageData* dst, const IntRect&);

The naming of these methods is misleading, I stared at 172     in->asUnmultipliedImage(imageData, drawingRect); before, wondering why you ignore the returned PassRefPtr<ImageData> until I realized there are two variants of the same method, one which returns void and the other with the PassRefPtr.

Can you rename the last two methods, to something like "convertToXXXImage" or maybe even a better name..
Also the "dst" parameter name is not needed.

> WebCore/platform/graphics/filters/FilterEffect.h:132
> +    inline void copyImageBytes(ImageData* src, ImageData* dst, const IntRect&);

s/src/source/ s/dst/destination/
Comment 16 Dirk Schulze 2010-11-29 15:01:16 PST
Comment on attachment 74995 [details]
patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=74995&action=review

Great patch. Just some minor notes. Could you write a manual perf test which shows the affect of your changes please? You can place the test in WebCore/manual-test. If it could measure the timings it would be a great tool for performance patches in the future, but it doesn't need it. Beth wrote a perf test for SVG clipping. It is placed there too. Maybe you can take a look at it.

> WebCore/platform/graphics/filters/FilterEffect.cpp:25
> +

IIRC we don't add newlines here.

>> WebCore/platform/graphics/filters/FilterEffect.cpp:106
>> +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst, const IntRect& rect)
> 
> Avoid abbrevations, while it is convienent here, I'd prefer source and destination.

I guess you're following the style in ImageBuffer*.cpp, but maybe Niko is right here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:110
> +    if (rect.x() < 0 || rect.y() < 0 || (rect.x() + rect.width()) > m_absolutePaintRect.width() || (rect.y() + rect.height()) > m_absolutePaintRect.height())

(rect.x() + rect.width())
See Nikos comment about right(), the same for the vertical position, just bottom()

>> WebCore/platform/graphics/filters/FilterEffect.cpp:159
>> +            unsigned char* end = src + (m_absolutePaintRect.width() * m_absolutePaintRect.height() * 4);
> 
> Again, src -> source, dst -> destination etc.

Would mean to write copyImageBytes(m_unmultipliedImageResult.get(), dst, rect); twice. Bu maybe it's still better to read. So...
Comment 17 Zoltan Herczeg 2010-12-06 00:30:23 PST
> > WebCore/platform/graphics/filters/FilterEffect.h:58
> > +    PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&);
> > +    PassRefPtr<ImageData> asPremultipliedImage(const IntRect&);
> > +    void asUnmultipliedImage(ImageData* dst, const IntRect&);
> > +    void asPremultipliedImage(ImageData* dst, const IntRect&);

Renamed to copyUnmultipliedImage and copyPremultipliedImage.

About the renamings, bottoms(), etc.: As Krit said, they all come from the GraphicsContext (platform dependent) implementation, and perhaps they should be renamed there as well (they probably copied the mac code). Unfortunately there are lots of platforms.
Comment 18 Dirk Schulze 2010-12-06 00:47:34 PST
(In reply to comment #17)
> > > WebCore/platform/graphics/filters/FilterEffect.h:58
> > > +    PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&);
> > > +    PassRefPtr<ImageData> asPremultipliedImage(const IntRect&);
> > > +    void asUnmultipliedImage(ImageData* dst, const IntRect&);
> > > +    void asPremultipliedImage(ImageData* dst, const IntRect&);
> 
> Renamed to copyUnmultipliedImage and copyPremultipliedImage.
> 
> About the renamings, bottoms(), etc.: As Krit said, they all come from the GraphicsContext (platform dependent) implementation, and perhaps they should be renamed there as well (they probably copied the mac code). Unfortunately there are lots of platforms.

Not your problem now. Just fix it for your patch and just for SVG. If you want, you can change the style in ImageBuffer* and GraphicsContext* in another patch afterwards.
Comment 19 Zoltan Herczeg 2010-12-06 05:28:11 PST
Created attachment 75679 [details]
patch 3

Added a nice manual test. 73% speedup with my patch (on a mac mini).
Comment 20 Dirk Schulze 2010-12-06 22:47:30 PST
Comment on attachment 75679 [details]
patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=75679&action=review

The patch looks great, still some notes from me.

> WebCore/platform/graphics/filters/FEBlend.cpp:92
> +    if (hasResult())
> +        return;

You should mention this in the changelog, that you do not create the same filter effect result twice.

> WebCore/platform/graphics/filters/FETurbulence.cpp:329
> +    if (!absolutePaintRect().width() || !absolutePaintRect().height())

do we allow negative width or height? If not you could use isEmpty() here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:130
> +    int xOrigin = rect.x();
> +    int xDest = 0;
> +    if (xOrigin < 0) {
> +        xDest = -xOrigin;
> +        xOrigin = 0;
> +    }
> +    int xEnd = rect.right();
> +    if (xEnd > m_absolutePaintRect.width())
> +        xEnd = m_absolutePaintRect.width();
> +
> +    int yOrigin = rect.y();
> +    int yDest = 0;
> +    if (yOrigin < 0) {
> +        yDest = -yOrigin;
> +        yOrigin = 0;
> +    }
> +    int yEnd = rect.bottom();
> +    if (yEnd > m_absolutePaintRect.height())
> +        yEnd = m_absolutePaintRect.height();

I wounder if we could use the intersect functionality of FloatRect instead doing it ourself here.
Comment 21 Zoltan Herczeg 2010-12-07 00:45:57 PST
> I wounder if we could use the intersect functionality of FloatRect instead doing it ourself here.

Don't think so as we need the info 'how' the intersection happens.
Comment 22 Zoltan Herczeg 2010-12-07 01:41:50 PST
Created attachment 75793 [details]
patch 4

Minor changes. Hopefully this is the last iteration.
Comment 23 WebKit Review Bot 2010-12-07 08:38:30 PST
Attachment 75793 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 WebKit Review Bot 2010-12-07 09:39:39 PST
Attachment 75793 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 WebKit Review Bot 2010-12-07 10:40:50 PST
Attachment 75793 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 WebKit Review Bot 2010-12-07 11:42:04 PST
Attachment 75793 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 WebKit Review Bot 2010-12-07 21:32:11 PST
Attachment 75793 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Dirk Schulze 2010-12-12 11:06:22 PST
Sorry, I reviewed the patch a while ago, somehow the review was not commited and I didn't check it afterwards :-(
Comment 29 Dirk Schulze 2010-12-12 11:13:30 PST
Comment on attachment 75793 [details]
patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=75793&action=review

r=me. Sorry for the delay. Just ping me, write me a mail if you are waiting more than a week.

> WebCore/ChangeLog:18
> +        Filters do not generate their results twice (or more), when

..twice (or more) anymore. ...
Comment 30 Zoltan Herczeg 2010-12-12 12:20:05 PST
> r=me. Sorry for the delay. Just ping me, write me a mail if you are waiting more than a week.

No poblem. I thought you were busy, and I had other task to do anyway.
Comment 31 Zoltan Herczeg 2010-12-13 02:45:51 PST
Landed in r73894
Closing bug.