Passing alpha to DeferredImageDecoder once decoding completes

• Component: New Bugs
• Status: RESOLVED FIXED

[Min Qin]

Passing alpha to DeferredImageDecoder once decoding completes

[Min Qin]

Created attachment 186672 [details]
Patch

[WebKit Review Bot]

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

[Hin-Chung Lam]

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();

[Min Qin]

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?

[Hin-Chung Lam]

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.

[Min Qin]

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.

[Hin-Chung Lam]

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.

[Min Qin]

Created attachment 187898 [details]
Patch

[Min Qin]

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.

[Hin-Chung Lam]

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..)

[Min Qin]

Created attachment 188163 [details]
Patch

[Min Qin]

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.

[Hin-Chung Lam]

Okay. LGTM.

[Min Qin]

ping. Stephen, any thoughts on this.

[Stephen White]

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?

[Min Qin]

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.

[Min Qin]

(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.

[Stephen White]

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.

[Min Qin]

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.

[Stephen White]

(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.

[Hin-Chung Lam]

I love the fact that you guys are discussing locking, atomic read/write in this thread. :D

[WebKit Review Bot]

Comment on attachment 188163 [details]
Patch

Clearing flags on attachment: 188163

Committed r142891: <http://trac.webkit.org/changeset/142891>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tom Hudson]

If there's some chance the mutex isn't necessary, I wish we hadn't landed this.

[Min Qin]

(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.

[Hin-Chung Lam]

(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.

[Min Qin]

(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?

[Hin-Chung Lam]

(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.

[Min Qin]

(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.

[Hin-Chung Lam]

No, it won't change from false to true.