Bug 108892 - Passing alpha to DeferredImageDecoder once decoding completes
Summary: Passing alpha to DeferredImageDecoder once decoding completes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-04 20:00 PST by Min Qin
Modified: 2013-02-14 11:47 PST (History)
8 users (show)

See Also:


Attachments
Patch (13.65 KB, patch)
2013-02-05 12:07 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (8.94 KB, patch)
2013-02-12 10:52 PST, Min Qin
no flags Details | Formatted Diff | Diff
Patch (9.69 KB, patch)
2013-02-13 13:23 PST, Min Qin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Min Qin 2013-02-04 20:00:19 PST
Passing alpha to DeferredImageDecoder once decoding completes
Comment 1 Min Qin 2013-02-05 12:07:44 PST
Created attachment 186672 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-05 12:33:38 PST
Comment on attachment 186672 [details]
Patch

Attachment 186672 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373796
Comment 3 Hin-Chung Lam 2013-02-11 15:58:22 PST
Comment on attachment 186672 [details]
Patch

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

You don't need to change LazyDecodingPixel or ScaledImageFragment.

Save the alpha state in ImageFrameGenerator instead. You will have to lock it when accessing this field.

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:185
> +        LazyDecodingPixelRef* pixelRef = static_cast<LazyDecodingPixelRef*>(m_lazyDecodedFrame.getSkBitmap().pixelRef());

alpha state should be saved in ImageFrameGenerator.

ImageFrameGenerator corresponds to one image file. It exists regardless whether or not an image is cached or not. So in the case of ScaledImageFragment being evicted the code can return the correct alpha state regardless of whether it is cached or not.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);

Do something like:

m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();
Comment 4 Min Qin 2013-02-12 09:50:14 PST
Comment on attachment 186672 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
>> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);
> 
> Do something like:
> 
> m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();

hmm.... if isComplete is false, shouldn't we return true there to force image decoding?
Comment 5 Hin-Chung Lam 2013-02-12 09:55:39 PST
Comment on attachment 186672 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
>>> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);
>> 
>> Do something like:
>> 
>> m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();
> 
> hmm.... if isComplete is false, shouldn't we return true there to force image decoding?

I don't know what you mean.

Actually now I think about it again you can just do !fullSizeBitmap.isOpaque(). Don't call frameHasAlphaAtIndex(0) it might have side effect.
Comment 6 Min Qin 2013-02-12 10:02:20 PST
Comment on attachment 186672 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
>>>> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);
>>> 
>>> Do something like:
>>> 
>>> m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();
>> 
>> hmm.... if isComplete is false, shouldn't we return true there to force image decoding?
> 
> I don't know what you mean.
> 
> Actually now I think about it again you can just do !fullSizeBitmap.isOpaque(). Don't call frameHasAlphaAtIndex(0) it might have side effect.

Yes, I think !fullSizeBitmap.isOpaque() should be correct.
hasAlpha should be defaulted to true. So if isComplete is false, we should use true so that there are no optimization happening elsewhere.
Comment 7 Hin-Chung Lam 2013-02-12 10:04:02 PST
Comment on attachment 186672 [details]
Patch

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

>>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
>>>>> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);
>>>> 
>>>> Do something like:
>>>> 
>>>> m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();
>>> 
>>> hmm.... if isComplete is false, shouldn't we return true there to force image decoding?
>> 
>> I don't know what you mean.
>> 
>> Actually now I think about it again you can just do !fullSizeBitmap.isOpaque(). Don't call frameHasAlphaAtIndex(0) it might have side effect.
> 
> Yes, I think !fullSizeBitmap.isOpaque() should be correct.
> hasAlpha should be defaulted to true. So if isComplete is false, we should use true so that there are no optimization happening elsewhere.

isOpaque() is default to false, it is set to true only if decoder tells it to. So it's more accurate than mixing it with isComplete. For example progressive JPEG, an image can be partially decoded (i.e. isComplete == false) but it is opaque.
Comment 8 Min Qin 2013-02-12 10:52:19 PST
Created attachment 187898 [details]
Patch
Comment 9 Min Qin 2013-02-12 10:52:47 PST
Comment on attachment 186672 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:185
>> +        LazyDecodingPixelRef* pixelRef = static_cast<LazyDecodingPixelRef*>(m_lazyDecodedFrame.getSkBitmap().pixelRef());
> 
> alpha state should be saved in ImageFrameGenerator.
> 
> ImageFrameGenerator corresponds to one image file. It exists regardless whether or not an image is cached or not. So in the case of ScaledImageFragment being evicted the code can return the correct alpha state regardless of whether it is cached or not.

Done.

>>>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:213
>>>>>> +    bool hasAlpha = (*decoder)->frameHasAlphaAtIndex(0);
>>>>> 
>>>>> Do something like:
>>>>> 
>>>>> m_hasAlpha = isComplete && !fullSizeBitmap.isOpaque();
>>>> 
>>>> hmm.... if isComplete is false, shouldn't we return true there to force image decoding?
>>> 
>>> I don't know what you mean.
>>> 
>>> Actually now I think about it again you can just do !fullSizeBitmap.isOpaque(). Don't call frameHasAlphaAtIndex(0) it might have side effect.
>> 
>> Yes, I think !fullSizeBitmap.isOpaque() should be correct.
>> hasAlpha should be defaulted to true. So if isComplete is false, we should use true so that there are no optimization happening elsewhere.
> 
> isOpaque() is default to false, it is set to true only if decoder tells it to. So it's more accurate than mixing it with isComplete. For example progressive JPEG, an image can be partially decoded (i.e. isComplete == false) but it is opaque.

Done.
Comment 10 Hin-Chung Lam 2013-02-12 21:34:43 PST
Comment on attachment 187898 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:186
> +        return pixelRef->frameGenerator()->hasAlpha();

DeferredImageDecoder already has a pointer to ImageFrameGenerator.

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.h:70
> +    bool hasAlpha() const { return m_hasAlpha; }

This is not thread-safe and we need a lock for reading and writing to this value.

You don't want to acquire the decodeMutex.. so I guess it's fine to have a alphaMutex (reluctantly..)
Comment 11 Min Qin 2013-02-13 13:23:42 PST
Created attachment 188163 [details]
Patch
Comment 12 Min Qin 2013-02-13 13:26:25 PST
Comment on attachment 187898 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:186
>> +        return pixelRef->frameGenerator()->hasAlpha();
> 
> DeferredImageDecoder already has a pointer to ImageFrameGenerator.

Done.

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.h:70
>> +    bool hasAlpha() const { return m_hasAlpha; }
> 
> This is not thread-safe and we need a lock for reading and writing to this value.
> 
> You don't want to acquire the decodeMutex.. so I guess it's fine to have a alphaMutex (reluctantly..)

Good catch on thread safety.
Comment 13 Hin-Chung Lam 2013-02-13 13:33:54 PST
Okay. LGTM.
Comment 14 Min Qin 2013-02-13 15:05:55 PST
ping. Stephen, any thoughts on this.
Comment 15 Stephen White 2013-02-13 17:03:00 PST
Comment on attachment 188163 [details]
Patch

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

OK.  r=me

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:227
> +    MutexLocker lock(m_alphaMutex);
> +    return m_hasAlpha;

I know I'll regret asking this, since threading has a long history of making me look like an idiot, but why do you need to lock the mutex for a read?  It's a bool, so all reads should be atomic anyway, no?
Comment 16 Min Qin 2013-02-13 18:18:48 PST
Comment on attachment 188163 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:227
>> +    return m_hasAlpha;
> 
> I know I'll regret asking this, since threading has a long history of making me look like an idiot, but why do you need to lock the mutex for a read?  It's a bool, so all reads should be atomic anyway, no?

reading a bool should be atomic, but I am not sure whether this is the case with multi-core processor cache.
If read/write are on different processors, this probably might not hold.
Comment 17 Min Qin 2013-02-13 20:15:47 PST
(In reply to comment #16)
> (From update of attachment 188163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188163&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:227
> >> +    return m_hasAlpha;
> > 
> > I know I'll regret asking this, since threading has a long history of making me look like an idiot, but why do you need to lock the mutex for a read?  It's a bool, so all reads should be atomic anyway, no?
> 
> reading a bool should be atomic, but I am not sure whether this is the case with multi-core processor cache.
> If read/write are on different processors, this probably might not hold.

In addition to processor cache, there could be other factors that cause out-of-order execution: pipelining, cache, optimization etc. In such cases, when we set the boolean to false, the code before the set statement might not have been executed yet.
Comment 18 Stephen White 2013-02-14 03:39:20 PST
Comment on attachment 188163 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:227
>>>> +    return m_hasAlpha;
>>> 
>>> I know I'll regret asking this, since threading has a long history of making me look like an idiot, but why do you need to lock the mutex for a read?  It's a bool, so all reads should be atomic anyway, no?
>> 
>> reading a bool should be atomic, but I am not sure whether this is the case with multi-core processor cache.
>> If read/write are on different processors, this probably might not hold.
> 
> In addition to processor cache, there could be other factors that cause out-of-order execution: pipelining, cache, optimization etc. In such cases, when we set the boolean to false, the code before the set statement might not have been executed yet.

Looks like I was wrong in the general case anyway:  although for the vast majority of CPU architectures, a single read or write of machine word or smaller is guaranteed atomic, this is still technically processor-specific, and not guaranteed by C++.

However, I don't understand what you mean by "the code before the set statement might not have executed yet".  Do you mean, the call to !fullSizeBitmap.isOpaque()?  That seems to be the only thing that the mutex protects:  that m_hasAlpha is set to the value of !fullSizeBitmap.isOpaque() atomically.  The call to hasAlpha() could come in on another thread before or after that change to the value, and it might return true or false depending on thread ordering.  The only thing the mutex ensures is that it won't occur during !fullSizeBitmap.isOpaque() or the assignment.  If that's the intent, sorry for the tangent.
Comment 19 Min Qin 2013-02-14 08:48:36 PST
In out-of-order execution, if you have some code like:

do_sth_to_beta();
alpha=true;     

when alpha=true is executed, you cannot guarantee that do_sth_to_beta() is finished.
And on another thread, if you have

if(alpha) {
  beta=false;
}
This is problematic. Since do_sth_to_beta() could be still running, so setting beta to false will have an adverse effect on that call. 


(In reply to comment #18)
> 
> Looks like I was wrong in the general case anyway:  although for the vast majority of CPU architectures, a single read or write of machine word or smaller is guaranteed atomic, this is still technically processor-specific, and not guaranteed by C++.
> 
> However, I don't understand what you mean by "the code before the set statement might not have executed yet".  Do you mean, the call to !fullSizeBitmap.isOpaque()?  That seems to be the only thing that the mutex protects:  that m_hasAlpha is set to the value of !fullSizeBitmap.isOpaque() atomically.  The call to hasAlpha() could come in on another thread before or after that change to the value, and it might return true or false depending on thread ordering.  The only thing the mutex ensures is that it won't occur during !fullSizeBitmap.isOpaque() or the assignment.  If that's the intent, sorry for the tangent.
Comment 20 Stephen White 2013-02-14 09:34:01 PST
(In reply to comment #19)
> In out-of-order execution, if you have some code like:
> 
> do_sth_to_beta();
> alpha=true;     
> 
> when alpha=true is executed, you cannot guarantee that do_sth_to_beta() is finished.

I think you're confusing out-of-order execution with multithreading.  Out-of-order execution only means that 
instructions can be reordered on the processor if they have no effect on apparent program order.  I.e., you can hoist a read above an ALU op, as long as there are no data dependencies between them. The instructions are still retired in program order.  Cache coherence ensures that this holds across processors as well.

> And on another thread, if you have
> 
> if(alpha) {
>   beta=false;
> }
> This is problematic. Since do_sth_to_beta() could be still running, so setting beta to false will have an adverse effect on that call. 

Right, that would be problematic, but that's not the case here:  you only have the mutex protecting one write to the variable, and one read, not two writes.  As I said before, I don't think the mutex protecting the read is doing anything, but that is processor-dependent and harmless I guess.

Anyway, this is getting way off topic for the code review, and my r+ still stands.

> 
> (In reply to comment #18)
> > 
> > Looks like I was wrong in the general case anyway:  although for the vast majority of CPU architectures, a single read or write of machine word or smaller is guaranteed atomic, this is still technically processor-specific, and not guaranteed by C++.
> > 
> > However, I don't understand what you mean by "the code before the set statement might not have executed yet".  Do you mean, the call to !fullSizeBitmap.isOpaque()?  That seems to be the only thing that the mutex protects:  that m_hasAlpha is set to the value of !fullSizeBitmap.isOpaque() atomically.  The call to hasAlpha() could come in on another thread before or after that change to the value, and it might return true or false depending on thread ordering.  The only thing the mutex ensures is that it won't occur during !fullSizeBitmap.isOpaque() or the assignment.  If that's the intent, sorry for the tangent.
Comment 21 Hin-Chung Lam 2013-02-14 10:47:38 PST
I love the fact that you guys are discussing locking, atomic read/write in this thread. :D
Comment 22 WebKit Review Bot 2013-02-14 10:51:07 PST
Comment on attachment 188163 [details]
Patch

Clearing flags on attachment: 188163

Committed r142891: <http://trac.webkit.org/changeset/142891>
Comment 23 WebKit Review Bot 2013-02-14 10:51:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Tom Hudson 2013-02-14 10:54:15 PST
If there's some chance the mutex isn't necessary, I wish we hadn't landed this.
Comment 25 Min Qin 2013-02-14 11:01:54 PST
(In reply to comment #24)
> If there's some chance the mutex isn't necessary, I wish we hadn't landed this.


I am wondering whether there will be a case that:
if (decoder->frameHasAlphaAtIndex(0)) {
  read_pixelref();  
}
However, on another thread where alpha value just got set, the skbitmap still haven't been fully decoded yet. So we need a memory barrier there and Mutex should do that.
Comment 26 Hin-Chung Lam 2013-02-14 11:03:44 PST
(In reply to comment #25)
> (In reply to comment #24)
> > If there's some chance the mutex isn't necessary, I wish we hadn't landed this.
> 
> 
> I am wondering whether there will be a case that:
> if (decoder->frameHasAlphaAtIndex(0)) {
>   read_pixelref();  
> }
> However, on another thread where alpha value just got set, the skbitmap still haven't been fully decoded yet. So we need a memory barrier there and Mutex should do that.

That will not be the case. alpha value is known only after an image is fully decoded.
Comment 27 Min Qin 2013-02-14 11:10:40 PST
(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > If there's some chance the mutex isn't necessary, I wish we hadn't landed this.
> > 
> > 
> > I am wondering whether there will be a case that:
> > if (decoder->frameHasAlphaAtIndex(0)) {
> >   read_pixelref();  
> > }
> > However, on another thread where alpha value just got set, the skbitmap still haven't been fully decoded yet. So we need a memory barrier there and Mutex should do that.
> 
> That will not be the case. alpha value is known only after an image is fully decoded.

Would multi processor cache hurt?
Comment 28 Hin-Chung Lam 2013-02-14 11:27:14 PST
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (In reply to comment #24)
> > > > If there's some chance the mutex isn't necessary, I wish we hadn't landed this.
> > > 
> > > 
> > > I am wondering whether there will be a case that:
> > > if (decoder->frameHasAlphaAtIndex(0)) {
> > >   read_pixelref();  
> > > }
> > > However, on another thread where alpha value just got set, the skbitmap still haven't been fully decoded yet. So we need a memory barrier there and Mutex should do that.
> > 
> > That will not be the case. alpha value is known only after an image is fully decoded.
> 
> Would multi processor cache hurt?

I don't know what this means.

But reading Stephen's comments that get me thinking that mutex might not be necessary. The value is either true or false and it doesn't hurt to have alpha == true. So even if we don't protect it and assignment comes while decode() is running the worst case is alpha is true when it should be false, which is benign anyway.
Comment 29 Min Qin 2013-02-14 11:45:16 PST
(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > (In reply to comment #24)
> > > > > If there's some chance the mutex isn't necessary, I wish we hadn't landed this.
> > > > 
> > > > 
> > > > I am wondering whether there will be a case that:
> > > > if (decoder->frameHasAlphaAtIndex(0)) {
> > > >   read_pixelref();  
> > > > }
> > > > However, on another thread where alpha value just got set, the skbitmap still haven't been fully decoded yet. So we need a memory barrier there and Mutex should do that.
> > > 
> > > That will not be the case. alpha value is known only after an image is fully decoded.
> > 
> > Would multi processor cache hurt?
> 
> I don't know what this means.
> 
> But reading Stephen's comments that get me thinking that mutex might not be necessary. The value is either true or false and it doesn't hurt to have alpha == true. So even if we don't protect it and assignment comes while decode() is running the worst case is alpha is true when it should be false, which is benign anyway.

Yes, for this case we are probably fine since the starting value is always true, and will only be changed to false later on.
However, if there is a case that we can change the value from false to true, then it will be different. Processor A changes the value from false to true. At the same time processor B checks its own cache and sees that it is false, so it decides to do nothing. But since that is not going to happen, i think we are probably safe.
Comment 30 Hin-Chung Lam 2013-02-14 11:47:49 PST
No, it won't change from false to true.