| Summary: | [Haiku] Adding Image-specific files to WebCore. | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Maxime Simon <simon.maxime> |
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | eric, leavengood, pkasting |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | Other | ||
| Attachments: | |||
|
Description
Maxime Simon
2009-08-09 07:14:38 PDT
Created attachment 34420 [details]
Patch to add six image-specific files to WebCore/platform/graphics/.
Comment on attachment 34420 [details]
Patch to add six image-specific files to WebCore/platform/graphics/.
Please split these out. GraphicsContext and the ImageSource stuff really need their own changes, they're complicated. :) Also, I made comments about parts of this patch in the previous bug which were not addressed by this posting.
Created attachment 34566 [details]
Patch to add four image-specific files to WebCore/platform/graphics/.
Created attachment 34568 [details]
Patch to add GraphicsContext to WebCore/platform/graphics/haiku.
Created attachment 34570 [details]
Patch to add ImageSource to WebCore/platform/graphics/haiku.
Peter is our Image* guy these days. Comment on attachment 34568 [details]
Patch to add GraphicsContext to WebCore/platform/graphics/haiku.
Looks OK.
Comment on attachment 34566 [details]
Patch to add four image-specific files to WebCore/platform/graphics/.
It seems this is not needed:
Icon::Icon()
35 {
36 notImplemented();
37 }
Please remove it from both Icon.h and from Icon.cpp
Comment on attachment 34570 [details] Patch to add ImageSource to WebCore/platform/graphics/haiku. > diff --git a/WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp b/WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp > new file mode 100644 > index 0000000..ee2184c > --- /dev/null > +++ b/WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp > @@ -0,0 +1,222 @@> +// FIXME: Should use the Haiku translator API. As of yesterday, there's a cross-platform ImageSource.cpp in platform/graphics. It looks at a glance like you could just build with this file (except for one item I note below) instead of adding an ImageSourceHaiku.cpp. > +ImageDecoder* createDecoder(const Vector<char>& data) > +{ > + > + // JPEG > + // FIXME: Add JPEG support. You're using the cross-platform decoders for all other filetypes. Is there something wrong with the cross-platform JPEG decoder that prevents you from even instantiating it? (In reply to comment #9) > > +ImageDecoder* createDecoder(const Vector<char>& data) > > +{ > > + > > + // JPEG > > + // FIXME: Add JPEG support. > > You're using the cross-platform decoders for all other filetypes. Is there > something wrong with the cross-platform JPEG decoder that prevents you from > even instantiating it? Indeed, in the default Haiku distribution libjpeg isn't available. Of course we could add it as dependency, but the basic way to handle ImageDecoder in Haiku is to use BTranslators. And we intended to use it for all the other filetypes as well. I may have to discuss it with Ryan. Regards, Maxime (In reply to comment #10) > (In reply to comment #9) > > > +ImageDecoder* createDecoder(const Vector<char>& data) > > > +{ > > > + > > > + // JPEG > > > + // FIXME: Add JPEG support. > > > > You're using the cross-platform decoders for all other filetypes. Is there > > something wrong with the cross-platform JPEG decoder that prevents you from > > even instantiating it? > > Indeed, in the default Haiku distribution libjpeg isn't available. > Of course we could add it as dependency, but the basic way to handle > ImageDecoder in Haiku is to use BTranslators. And we intended to use it for all > the other filetypes as well. I may have to discuss it with Ryan. I see. Well, if you want to land the patch in the sort of state it's in now, we could either #if !PLATFORM(HAIKU) (or whatever the appropriate switch is) that block, or add some sort of ENABLE flag (which I am less in favor of). If you want to use a completely different mechanism (I have no idea what BTranslators are :D), we can design for that once you've got code that would work. (In reply to comment #11) > > I see. Well, if you want to land the patch in the sort of state it's in now, > we could either #if !PLATFORM(HAIKU) (or whatever the appropriate switch is) > that block, or add some sort of ENABLE flag (which I am less in favor of). Maxime let's just revert to using their ImageDecoder for now, and we can try using translators later. > If you want to use a completely different mechanism (I have no idea what > BTranslators are :D), we can design for that once you've got code that would > work. Maybe you really don't care, but: Translators are a pretty interesting idea from BeOS (that has been copied in Haiku) which allow file formats to be converted to a common base format to facilitate all kinds of conversions. It is sort of like translating languages into a base language to translate between them (though it works a lot better for binary data than human languages.) The main use at the moment is for images, so that all the image translators can convert to and from a basic bitmap, which allows conversions from PNG to JPG, JPG to GIF, RAW to PNG, etc. It is a pretty cool idea and of course we want to make use of such nice Haiku-only technologies in our WebKit port to make it truly native. The end result of this should be that our WebKit port would be able to render any images we have a translator for, without having to modify or recompile WebKit itself. We can come back to this later when we have something better working. Your willingness to adapt WebKit to help us is very much appreciated! (In reply to comment #12) > Translators are a pretty interesting idea from BeOS (that has been copied in > Haiku) which allow file formats to be converted to a common base format to > facilitate all kinds of conversions. It is sort of like translating languages > into a base language to translate between them (though it works a lot better > for binary data than human languages.) The main use at the moment is for > images, so that all the image translators can convert to and from a basic > bitmap, which allows conversions from PNG to JPG, JPG to GIF, RAW to PNG, etc. > > It is a pretty cool idea and of course we want to make use of such nice > Haiku-only technologies in our WebKit port to make it truly native. The end > result of this should be that our WebKit port would be able to render any > images we have a translator for, without having to modify or recompile WebKit > itself. Beware of the potential cost of that, if fitting it into WebCore involves e.g. copying an image's data, or doing more than one conversion. Decoding images has to be really, really fast in a web browser :) (In reply to comment #13) > > Beware of the potential cost of that, if fitting it into WebCore involves e.g. > copying an image's data, or doing more than one conversion. Decoding images > has to be really, really fast in a web browser :) Good point. There might be some overhead in using the translator system but hopefully not too much. We can do some benchmarks to be sure, but even if using the translators is a bit slower it might still be worth it. At least it shouldn't affect other ports ;) One other issue is that the translator system does not yet (and may never) support animated images. So for GIFs (and other future animated image formats, MNG, APNG, etc) we would have to fallback to using the WebKit decoders. But I'm tempted to try to fix that in Haiku...it is just tricky since translators are for any kind of data not just images. (In reply to comment #13) > > Beware of the potential cost of that, if fitting it into WebCore involves e.g. > copying an image's data, or doing more than one conversion. Decoding images > has to be really, really fast in a web browser :) Actually this has made me wonder: have you guys ever considered moving image loading and decoding into another thread? Maybe that would add unnecessary complexity and locking issues but at first glance it seems like something that would benefit from parallelism (multiple cores and all being common these days.) (In reply to comment #15) > Actually this has made me wonder: have you guys ever considered moving image > loading and decoding into another thread? Maybe that would add unnecessary > complexity and locking issues but at first glance it seems like something that > would benefit from parallelism (multiple cores and all being common these > days.) Not sure it'd be a big win since image decoding only happens on-demand, when the image is actually going to be drawn onscreen. Maybe for lots of images that are all visible at once. I haven't given it a lot of thought. Consider asking dhyatt on IRC. Comment on attachment 34568 [details] Patch to add GraphicsContext to WebCore/platform/graphics/haiku. Clearing flags on attachment: 34568 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog A WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp Committed r47147 M WebCore/ChangeLog A WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp r47147 = 898556fb5c9912a0b53f789ad3e50e439ce26999 (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/47147 All reviewed patches have been landed. Closing bug. Sorry for the early closure. We just hit bug 28230. Created attachment 34720 [details]
Adding four image-specific files to WebCore.
---
6 files changed, 386 insertions(+), 2 deletions(-)
Comment on attachment 34570 [details]
Patch to add ImageSource to WebCore/platform/graphics/haiku.
I marked this patch as obsolete, we will use ImageSource.cpp instead.
Tough I will post a new bug with a patch as the Haiku port doesn't support JPEG for now.
Comment on attachment 34720 [details]
Adding four image-specific files to WebCore.
Looks OK.
Comment on attachment 34720 [details] Adding four image-specific files to WebCore. Clearing flags on attachment: 34720 Committed r47463: <http://trac.webkit.org/changeset/47463> All reviewed patches have been landed. Closing bug. |