Bug 28130 – [Haiku] Adding Image-specific files to WebCore.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maxime Simon]

Here is a patch to add these six files which are specific to images (or related):
WebCore/platform/graphics/haiku/GraphicsContextHaiku.cpp
WebCore/platform/graphics/haiku/IconHaiku.cpp
WebCore/platform/graphics/haiku/ImageBufferData.h
WebCore/platform/graphics/haiku/ImageBufferHaiku.cpp
WebCore/platform/graphics/haiku/ImageHaiku.cpp
WebCore/platform/graphics/haiku/ImageSourceHaiku.cpp

Regards,
Maxime

[Maxime Simon]

Created attachment 34420 [details]
Patch to add six image-specific files to WebCore/platform/graphics/.

[Eric Seidel (no email)]

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.

[Maxime Simon]

Created attachment 34566 [details]
Patch to add four image-specific files to WebCore/platform/graphics/.

[Maxime Simon]

Created attachment 34568 [details]
Patch to add GraphicsContext to WebCore/platform/graphics/haiku.

[Maxime Simon]

Created attachment 34570 [details]
Patch to add ImageSource to WebCore/platform/graphics/haiku.

[Eric Seidel (no email)]

Peter is our Image* guy these days.

[Eric Seidel (no email)]

Comment on attachment 34568 [details]
Patch to add GraphicsContext to WebCore/platform/graphics/haiku.

Looks OK.

[Eric Seidel (no email)]

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

[Peter Kasting]

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?

[Maxime Simon]

(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

[Peter Kasting]

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

[Ryan Leavengood]

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

[Peter Kasting]

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

[Ryan Leavengood]

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

[Ryan Leavengood]

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

[Peter Kasting]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Sorry for the early closure.  We just hit bug 28230.

[Maxime Simon]

Created attachment 34720 [details]
Adding four image-specific files to WebCore.


---
 6 files changed, 386 insertions(+), 2 deletions(-)

[Maxime Simon]

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.

[Eric Seidel (no email)]

Comment on attachment 34720 [details]
Adding four image-specific files to WebCore.

Looks OK.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.