Bug 103555 – Make LazyDecodingPixelRef inherit from skia::LazyPixelRef so that cc thread can access it

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Min Qin]

Make LazyDecodingPixelRef inherit from skia::LazyPixelRef so that cc thread can access it

[Min Qin]

Created attachment 176559 [details]
Patch

[Hin-Chung Lam]

Is Skia rolled already?

[Min Qin]

The skia change i made is in rev 168995, and webkit now is using 169895.
So I assume the change has rolled already.

(In reply to comment #2)
> Is Skia rolled already?

[Hin-Chung Lam]

Comment on attachment 176559 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:94
> +    m_mutex.lock();

Use MutexLocker lock(m_mutex). The mutex just needs to protect access to m_frameGenerator and m_scaledSize. We don't absolutely need a mutex here because they don't mutate but just for safety.

[Min Qin]

Created attachment 176565 [details]
Patch

[Min Qin]

(In reply to comment #4)
> (From update of attachment 176559 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176559&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:94
> > +    m_mutex.lock();
> 
> Use MutexLocker lock(m_mutex). The mutex just needs to protect access to m_frameGenerator and m_scaledSize. We don't absolutely need a mutex here because they don't mutate but just for safety.

Done

[Hin-Chung Lam]

Comment on attachment 176565 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:95
> +    // TODO(hclam): check if only a particular rect is available in image cache.

Whoops also WebKit doesn't use TODO(XXX), it's just TODO. Sorry I didn't point it out earlier.

[Min Qin]

Created attachment 176569 [details]
Patch

[Min Qin]

(In reply to comment #7)
> (From update of attachment 176565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176565&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:95
> > +    // TODO(hclam): check if only a particular rect is available in image cache.
> 
> Whoops also WebKit doesn't use TODO(XXX), it's just TODO. Sorry I didn't point it out earlier.

ah.... Sorry, missed that. Fixed now

[Hin-Chung Lam]

lgtm from me by the way.

[Stephen White]

Comment on attachment 176569 [details]
Patch

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

OK.  r=me

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:103
> +    lockPixels();
> +    unlockPixels();

Not sure I really get this, since you're going to have to lock again to use them, but I'm sure you know that.  :)

[WebKit Review Bot]

Comment on attachment 176569 [details]
Patch

Clearing flags on attachment: 176569

Committed r136167: <http://trac.webkit.org/changeset/136167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 103701

[Min Qin]

Hi, Stephen, Can you help me recommit this patch?

A fix for the previous build fix is submitted in r170542
The current DEPS roll is at 171019 now.

Thanks

[Stephen White]

Comment on attachment 176569 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 176569 [details]
Patch

Clearing flags on attachment: 176569

Committed r136630: <http://trac.webkit.org/changeset/136630>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Reverted r136630 for reason:

Still breaking Chrome/Win Debug builds

Committed r136728: <http://trac.webkit.org/changeset/136728>

[Min Qin]

Comment on attachment 176569 [details]
Patch

Hi, Stephen, sorry about this, but can you approve this again. Thanks.

We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376

[Stephen White]

(In reply to comment #20)
> (From update of attachment 176569 [details])
> Hi, Stephen, sorry about this, but can you approve this again. Thanks.
> 
> We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376

Have you built windows debug (component build) locally?

[James Robinson]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 176569 [details] [details])
> > Hi, Stephen, sorry about this, but can you approve this again. Thanks.
> > 
> > We believe the windows dbg bot failure is caused by that the lazy_pixel_ref.h is not built into the dll. So we split the file into a .cc and .h file. The fix is committed in r171375. And DEPS has rolled to r171376
> 
> Have you built windows debug (component build) locally?

And if you aren't set up to do this, you can send a tryjob to the chromium win trybot (which is debug and component) to verify.  Please do one or the other before attempting to land.

[Min Qin]

Hi, Stephen. Yes, I tried our new patch on windows dbg build, and it compiles fine now.

[Stephen White]

Comment on attachment 176569 [details]
Patch

Great, thanks.  r=me

[WebKit Review Bot]

Comment on attachment 176569 [details]
Patch

Clearing flags on attachment: 176569

Committed r136891: <http://trac.webkit.org/changeset/136891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hin-Chung Lam]

Comment on attachment 176569 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:97
> +    return ImageDecodingStore::instance()->lockCompleteCache(m_frameGenerator.get(), m_scaledSize);

I don't think this is entirely correct, this method is used as a hint to see of the cache is there. So the code should be:

const ScaledImageFragment* cachedImage = ...lockCompleteCache();
if (!cachedImage)
  return false;
... unlockCache(cachedImage);
return true;

Can you make another change to fix this?

[Hin-Chung Lam]

(In reply to comment #27)
> (From update of attachment 176569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176569&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LazyDecodingPixelRef.cpp:97
> > +    return ImageDecodingStore::instance()->lockCompleteCache(m_frameGenerator.get(), m_scaledSize);
> 
> I don't think this is entirely correct, this method is used as a hint to see of the cache is there. So the code should be:
> 
> const ScaledImageFragment* cachedImage = ...lockCompleteCache();
> if (!cachedImage)
>   return false;
> ... unlockCache(cachedImage);
> return true;
> 
> Can you make another change to fix this?

If you don't do this it wil leak.. :(