Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
526153002:
Add ImageResource update bitmap images api (Closed)
|
Created:
6 years, 3 months ago by Noel Gordon Modified:
6 years, 3 months ago CC:
blink-reviews, jamesr, krit, jbroman, danakj, Rik, gavinp+loader_chromium.org, Stephen Chennney, Nate Chapin, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionAdd ImageResource update bitmap images api
The frames of bitmap images can change due to extrinsic reasons, other than new
network data arrival or image frame animations. Add a new ImageResoruce api for
updating images for extrinsic reasons, with optional decoder reset support used
to force image re-decoding if needed.
TEST=webkit_unit_tests --gtest_filter="ImageResource*UpdateBitmapImages"
BUG=410135, 369901
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181451
Patch Set 1 : #
Total comments: 2
Messages
Total messages: 25 (3 generated)
noel@chromium.org changed reviewers: + japhet@chromium.org
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... File Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... Source/core/fetch/ImageResource.h:84: static void updateBitmapImages(HashSet<ImageResource*>&, bool redecodeImages = false); I'm concerned about adding more bitmap-specific codepaths to ImageResource. Do SVG images not need this? This doesn't appear to be used in non-test code. Why don't we add it as part of the patch that uses it?
On 2014/09/03 03:05:12, pdr wrote: > https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... > File Source/core/fetch/ImageResource.h (right): > > https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... > Source/core/fetch/ImageResource.h:84: static void > updateBitmapImages(HashSet<ImageResource*>&, bool redecodeImages = false); > I'm concerned about adding more bitmap-specific codepaths to ImageResource. Do Why are you concerned? Bitmap images are intrinsic ImageResource, no? And ImageResource is responsible for creating them, no? > SVG images not need this? Does SVG use BitmapImages? > This doesn't appear to be used in non-test code. Why don't we add it as part of > the patch that uses it? Perhaps because the entire patch is a bit too large and cross-cutting to land in one go. Mr White suggested I break this part out and submit it per https://codereview.chromium.org/352873002/#msg11 and I concur with his advice. Tests are on https://codereview.chromium.org/266383003 and can't land them until the myriad other broken bits chromium are fixed. I'm itemizing those on https://code.google.com/p/chromium/issues/detail?id=35381 and fixing them and writing tests for each as I go.
Ahem ... > I'm itemizing those on > https://code.google.com/p/chromium/issues/detail?id=35381 and fixing them and > writing tests for each as I go. Ahem, I'm itemizing those on https://code.google.com/p/chromium/issues/detail?id=353818 ...
On 2014/09/03 04:15:53, Noel Gordon wrote: > Why are you concerned? Bitmap images are intrinsic ImageResource, no? And > ImageResource is responsible for creating them, no? What do you mean by "Bitmap images are intrinsic ImageResource"? > > This doesn't appear to be used in non-test code. Why don't we add it as part > of > > the patch that uses it? > > Perhaps because the entire patch is a bit too large and cross-cutting to land in > one go. Mr White suggested I break this part out and submit it per > https://codereview.chromium.org/352873002/#msg11 and I concur with his advice. I agree about splitting this up, I just didn't have the full context of that patch. As I worried, the full patch does not properly handle SVG images :(
On 2014/09/03 05:12:48, pdr wrote: > On 2014/09/03 04:15:53, Noel Gordon wrote: > > Why are you concerned? Bitmap images are intrinsic ImageResource, no? And > > ImageResource is responsible for creating them, no? > > What do you mean by "Bitmap images are intrinsic ImageResource"? ImageResource creates BitmapImages in response to data arriving from the network. That data arrival calls here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... and that calls into Imageresource::createImage() ... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... to create a BitmapImage. BitmapImage handles jpeg/pmg/gif/webp/bmp images, and ImageResource is tasked with the job of creating them, which is want I meant by intrinsic. ImageResource and BitmapImage are joined at the hip :) and have been for a very long time. I'm yet to find a need / use-case that would require their relationship to change.
On 2014/09/03 05:43:07, Noel Gordon wrote: > On 2014/09/03 05:12:48, pdr wrote: > > On 2014/09/03 04:15:53, Noel Gordon wrote: > > > Why are you concerned? Bitmap images are intrinsic ImageResource, no? And > > > ImageResource is responsible for creating them, no? > > > > What do you mean by "Bitmap images are intrinsic ImageResource"? > > ImageResource creates BitmapImages in response to data arriving from the > network. That data arrival calls here: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > and that calls into Imageresource::createImage() ... > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > to create a BitmapImage. > > BitmapImage handles jpeg/pmg/gif/webp/bmp images, and ImageResource is tasked > with the job of creating them, which is want I meant by intrinsic. > ImageResource and BitmapImage are joined at the hip :) and have been for a very > long time. I'm yet to find a need / use-case that would require their > relationship to change. ImageResource also handles SVG images.
On 2014/09/03 06:05:47, pdr wrote: Expanding a bit: ImageResource handles SVG images too, and these images need to work with color profiles too. For example, and SVG image itself can contain bitmap images that have color profiles.
Yes but means the SVG element that uses a BitmapImage is an ImageResource client, right?
On 2014/09/03 06:47:33, Noel Gordon wrote: > Yes but means the SVG element that uses a BitmapImage is an ImageResource > client, right? It is. If you could just add a test for bitmap images in SVG in your next patch, I'm a happy camper.
On 2014/09/03 16:45:25, pdr wrote: > On 2014/09/03 06:47:33, Noel Gordon wrote: > > Yes but means the SVG element that uses a BitmapImage is an ImageResource > > client, right? > > It is. Exactly. If SVG uses a BitmapImage it must become an ImageResource client so the resource can inform the client of changes to the image (loading, loaded, decoded) via image-changed events. ImageResource updateImage() does this by calling ImageResource::NotifyObservers. The current patch pushes multiple BitmapImages through that path to update all their clients. Indeed, any page resource that uses a BitmapImage, for example HTML, SVG or <canvas> elements, a CSS background / border image, a CSS style-generated image such as a cross-fade image, renderImage, and so on, must provide an image-changed handler. If they don't, that can't ever draw the image. Not really relevant to this patch, but do you know of any SVG element that use BitmapImage that do not respond to image-changed events properly?
On 2014/09/03 16:45:25, pdr wrote: > On 2014/09/03 06:47:33, Noel Gordon wrote: > > Yes but means the SVG element that uses a BitmapImage is an ImageResource > > client, right? > > It is. If you could just add a test for bitmap images in SVG in your next patch, > I'm a happy camper. https://codereview.chromium.org/266383003 has three SVG tests. I can't land those until more patches have landed ... On 2014/09/03 05:12:48, pdr wrote: > On 2014/09/03 04:15:53, Noel Gordon wrote: > > one go. Mr White suggested I break this part out and submit it per > > https://codereview.chromium.org/352873002/#msg11 and I concur with his advice. > > I agree about splitting this up, Indeed, it's a bit cross-cutting (part skia, part blink, and there's another patch of similar size for the chromium side too). One advantage of splitting it up is that I get to write tests for each part. Of the split out parts I've submitted for blink so far, I've found lots of untested / broken code, which is sad, so I fixed and added test coverage as I went along.
On 2014/09/03 05:12:48, pdr wrote: > I just didn't have the full context of that patch. Yeah understood, and why I wrote the larger [wip] to provide that fuller context to help the many folks I need to collaborate with in chrome to get this work done. Lots know their part of the system, but if I take them into other parts of chrome, skia, or blink, their knowledge of those parts is often minimal. That's fine and understandable: chrome is a large software system. Hard to give a full context to folks unless they are familiar with the many moving parts (OOPIF, impl-side-painting, deferred image decoding, YUV jpeg decoding, skPaint, ganesh, GPU fragment shaders, blink RenderLayer, ImageBuffer, GraphicsContext, gfx::Screen and gfx::Display, the RWH::WasResized IPC, a long and incomplete list btw) that are in-play here. > As I worried, the full patch does not properly handle SVG images :( :) image-change events are your friend, per #13, and my question to you there still stands - orthogonal to the current patch, but are there SVG elements that don't properly respond to image-changed events?
On 2014/09/03 06:13:53, pdr wrote: > ImageResource handles SVG images too, and these images need to work with color > profiles too. "ImageResource handles SVG images too", as in the SVG mimetype, right? Agree with that, but does this mimetype have a color profile at all, or one that differs from CSS default color profile (sRGB)?
On 2014/09/04 15:49:01, Noel Gordon wrote: > On 2014/09/03 06:13:53, pdr wrote: > > ImageResource handles SVG images too, and these images need to work with color > > profiles too. > > "ImageResource handles SVG images too", as in the SVG mimetype, right? Agree > with that, but does this mimetype have a color profile at all, or one that > differs from CSS default color profile (sRGB)? I'm not familiar with color profiles in SVG, but the spec is at https://svgwg.org/svg2-draft/single-page.html#color-ColorProfileDescriptions As far as I know, SVG images all respond to imageChangedEvents properly. The issue I was worried about in #4 was the case of an SVG image with an embedded (base64'd) bitmap image that has a color profile. This, strangely, is common because Adobe illustrator outputs base64'd images in SVG frequently (e.g., drop shadows). I do not think the deviceColorProfileChanged will work in this case.
On 2014/09/04 17:47:18, pdr wrote: > > I'm not familiar with color profiles in SVG, but the spec is at > https://svgwg.org/svg2-draft/single-page.html#color-ColorProfileDescriptions Thanks for the ref; it's similar to -webkit-color-profile which was removed from chrome. > As far as I know, SVG images all respond to imageChangedEvents properly. One case I've found was an SVG <pattern> that used a BitmapImage as the source image used to build the pattern. Sometimes the pattern does not update on an image-changed event. Not sure why yet. > issue I was worried about in #4 was the case of an SVG image with an embedded > (base64'd) bitmap image that has a color profile. This, strangely, is common > because Adobe illustrator outputs base64'd images in SVG frequently (e.g., drop > shadows). I do not think the deviceColorProfileChanged will work in this case. If base64'd means a base64 dataURL for a png/jpeg etc, that would work.
Thank you for filing https://code.google.com/p/chromium/issues/detail?id=411183. This patch: LGTM.
Just a minor nit. https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... File Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... Source/core/fetch/ImageResource.h:84: static void updateBitmapImages(HashSet<ImageResource*>&, bool redecodeImages = false); Can you change this to updateBitmapImagesForColorCorrection (or similar)?
On 2014/09/05 07:02:37, pdr wrote: > Thank you for filing https://code.google.com/p/chromium/issues/detail?id=411183. Yeah, base64'd does mean a base64 dataURL, so I'll write a layout test for that case when we get there. > This patch: LGTM.
On 2014/09/05 07:04:38, pdr wrote: > Just a minor nit. > > https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... > File Source/core/fetch/ImageResource.h (right): > > https://codereview.chromium.org/526153002/diff/40001/Source/core/fetch/ImageR... > Source/core/fetch/ImageResource.h:84: static void > updateBitmapImages(HashSet<ImageResource*>&, bool redecodeImages = false); > Can you change this to updateBitmapImagesForColorCorrection (or similar)? ImageResoruce doesn't really need to know why it has been told to update these images, it just does it, so we agree off-line that we're ok with the name as is. Thanks for the questions, and hope I've given you enough context. SVG <pattern> bugs are a separate issue, there are bugs there, and layout tests will reveal them. I notice it while doing prelimary work on https://code.google.com/p/chromium/issues/detail?id=334612 and will eventually get answers from the folks there.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noel@chromium.org/526153002/40001
Message was sent while issue was closed.
Committed patchset #1 (id:40001) as 181451 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
