Tomasz Kołodziejski [:tomasz] Reporter
	Description • 10 years ago

Especially when browser.newtab.preload is false. Try opening new tabs one by one quickly to see the blinking.

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 1 • 10 years ago

We're doing unnecessary conversions from base64 encoded logo to ArrayBuffer and then back to URI. I'll get rid of it.

	Tim Taubert [:ttaubert] (inactive)
	Comment 2 • 10 years ago

Tom, can you estimate the effort here, please?

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 3 • 10 years ago

This patch is removing unnecessary conversion from dataURI to ArrayBuffer to dataURI. Looks like it's better now.

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 4 • 10 years ago

I know this code so for me let's say 2. If I didn't know it I'd say possibly 3 to dig through all this stuff. I added my original work bug to as dependency of this one.

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 5 • 10 years ago

It looks like the usage of ArrayBuffers was intentional...

	Tomasz Kołodziejski [:tomasz] Reporter
	Comment 6 • 10 years ago

I'm still not sure about that related bug. Right now we're converting: base64 image => ArrayBuffer => Blob => objectURL. What was the reason that this was faster than just passing and displaying base64 image?

	Robert Kaiser
	Comment 7 • 10 years ago

This sounds like a cosmetic thing that we don't necessarily need to do manual QA on.

	Tim Taubert [:ttaubert] (inactive)
	Comment 8 • 10 years ago

Comment on attachment 8499253 [details] [diff] [review]
simplify-icon-handling.patch

Review of attachment 8499253 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should until Drew is back.

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 10 years ago

*we should wait

	Drew Willcoxon :adw
	Comment 10 • 10 years ago

Tom and I talked about this in person.  I think this must have something to do with decoding blobs vs. decoding data URIs.  I'm guessing that for images whose src's are blob URIs, the data in the blobs is not decoded until the images become visible, but for images whose src's are data URIs, the decoding isn't deferred that way.  Or perhaps both are deferred, but blobs are decoded off the main thread, and data URIs are decoded on the main thread.  Either way, I'm guessing, there's some window of time where for images backed by blobs, the image isn't decoded yet, leading to blinking.

Also, Alex is running into a problem in bug 1029889 that may be solved by switching back to data URIs instead of array buffers, so we may end up taking this bug regardless.

I'd like to track down my encoding idea and wait for the outcome in bug 1029889 before reviewing this.  I expect those things to happen in the next day or two, so I won't remove this bug from the current iteration.  If we're late in the iteration and this is still blocked, then we should remove it.

	Drew Willcoxon :adw
	Comment 11 • 10 years ago

Benoit, do you have any ideas about what we're seeing here?  It seems to me to be related to how images are decoded from blob URLs vs. data URIs.

We have a page, about:newtab, that loads in a browser in the hidden window.  It has a div with a background-image that's a blob URL.  When you open a new tab, we swap its docshell with the docshell of the hidden browser.  Sometimes when the hidden page is swapped and becomes visible, the background-image is absent for a split second, and then it pops in -- as if the image has not been decoded immediately.

If we encode the image in a data URI and set that as the background-image instead, then the image is always visible immediately.

I'm guessing this is either an image decoding thing, or a difference between how blobs and data URIs stream their data.  Something like, decoding the image from the blob is deferred until the image becomes visible, while decoding it from the data URI is not deferred.  Or the image decoding -- or data streaming -- happens off the main thread for the blob, but not for the data URI.  It must be something low level like that, because I think we've ruled out other possible causes.

	Benoit Girard (:BenWa)
	Comment 12 • 10 years ago

This sounds like intended behavior with image locking. :tn can give a better answer however.

	Timothy Nikkel (:tnikkel)
	Comment 13 • 10 years ago

I can't think of anywhere in imagelib or layout where a data uri would make a difference versus a blob for any decoding related decision, except one place: when we validate an image cache entry we always say that a data uri is valid immediately (so they don't have to validate and we just use the existing image), blobs have no such analog. The code for that is here http://mxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp?rev=2846ff134282#1619 you can just comment out that early return to see if that is having an effect for you. If it's not that my next suspicion would be a difference in how necko is delivering the data to imagelib.

	Drew Willcoxon :adw
	Comment 14 • 10 years ago

Thanks Timothy.  Commenting out the early return didn't have any effect: if the image was a data URI, it was still visible immediately.

A couple of other things I should have mentioned.  The blink only happens maybe 30% of the time.  And sometimes I noticed the blinking even when I closed another tab and then the about:newtab became the selected and visible tab.  i.e., I opened new tab 1, opened new tab 2, closed new tab 2, and then when new tab 1 became visible, the image on it blinked.

Do you know who we might ask about necko?  I did actually look at the data protocol and streams code a little.  Data URIs are base-64 decoded immediately and on the main thread, but after that, the decoded data is read on a subsequent turn of the event loop, just like with blobs, as far as I can tell.

	Timothy Nikkel (:tnikkel)
	Comment 15 • 10 years ago

(In reply to Drew Willcoxon :adw (Away 11/5–11/7) from comment #14)
> Thanks Timothy.  Commenting out the early return didn't have any effect: if
> the image was a data URI, it was still visible immediately.

Thanks, at least we ruled that out.

> A couple of other things I should have mentioned.  The blink only happens
> maybe 30% of the time.  And sometimes I noticed the blinking even when I
> closed another tab and then the about:newtab became the selected and visible
> tab.  i.e., I opened new tab 1, opened new tab 2, closed new tab 2, and then
> when new tab 1 became visible, the image on it blinked.

Hmm, that would imply not a networking thing as all the data should be stored on the image already.

Do you have a testcase that I could use to do some basic checks about what is going on? Webpage, or patch form would be fine.

	Seth Fowler [:seth] [:s2h]
	Comment 16 • 10 years ago

I just had a conversation with the Shumway folks about this issue, since they've seen something similar. This may be relevant.

They showed me a demo with code like this:

> var img = new Image();
> var canvas = document.body.appendChild(document.createElement('canvas'));
> var ctx = canvas.getContext('2d');
> ctx.fillRect(0, 0, 50, 50);
> img.src = canvas.toDataURL();
> ctx.drawImage(img, 50, 50);

This works; the image draws successfully. Note that they didn't wait for the image's 'load' event to fire or for the event loop to spin.

They then showed me a second demo using blob URIs (only showing changed code here, and I'm typing this without the original code so hopefully I get it right):

> canvas.toBlob(function (blob) {
>  img.src = URL.createObjectURL(blob);
>  ctx.drawImage(img, 50, 50);
> });

This does *not* work. You can't draw the blob URI without waiting for the image's 'load' event to fire.

Do you think it's possible that our newtab code may not be waiting for the image's 'load' event?

Even if it is, maybe the different behavior in these two cases provides a clue to what may be different about the behavior of blob URIs that we're seeing in this bug.

	Timothy Nikkel (:tnikkel)
	Comment 17 • 10 years ago

One more random guess, try commenting out all statements in image/src/RasterImage.cpp that set mInDecoder to true.

	Timothy Nikkel (:tnikkel)
	Comment 18 • 10 years ago

mInDecoder has been removed now (on nightly) so if that was the cause this bug should be fixed on nightly now.

	Drew Willcoxon :adw
	Comment 19 • 10 years ago

Sorry the needinfo has been open for so long.  I thought I'd have time to get back to this but I really haven't, and I don't know when I'll have time, so I'll clear the needinfo for now.  It's no excuse, but this bug is probably less of a priority now because about:newtab doesn't even show these logos anymore with the new "one-off" search UI, which is enabled by default.