Bug 27561

see bug 26467

A few comments: * Can you break this into distinct patches for the different pieces here? This patch is very large and hard to review. * You don't include ImageFrameSink.*, so no one can actually compile or use this. * "#if PLATFORM (WINCE)" and similar all over, especially with "#else <existing code>", really sucks for readability. Making this worse is the fact that frequently the two blocks of code are very similar or even share many common parts, and differ only slightly. A combination of helper functions and subclasses seems like it could do this much more readably. * goto? Really? It doesn't look necessary at all (a helper function plus a "continue;" would have served just as well). Overall it seems like these capabilities should be added to the existing code in a way that blends more seamlessly and is usable by any port. If I can manage to get back to working on Image system cleanup, maybe we can find a way of doing that stepwise.

(In reply to comment #2) > A few comments: > > * Can you break this into distinct patches for the different pieces here? This > patch is very large and hard to review. > > * You don't include ImageFrameSink.*, so no one can actually compile or use > this. Those are asking the opposite things... It was broken up to do the sink part separately. > * "#if PLATFORM (WINCE)" and similar all over, especially with "#else <existing > code>", really sucks for readability. Making this worse is the fact that > frequently the two blocks of code are very similar or even share many common > parts, and differ only slightly. A combination of helper functions and > subclasses seems like it could do this much more readably. > > * goto? Really? It doesn't look necessary at all (a helper function plus a > "continue;" would have served just as well). > > Overall it seems like these capabilities should be added to the existing code > in a way that blends more seamlessly and is usable by any port. If I can > manage to get back to working on Image system cleanup, maybe we can find a way > of doing that stepwise. I think it needs more cleanup too. I don't think we have any intention of making our code potentially less fast for the sake of readability though, and we don't want to risk breaking anything for others. There is a balancing act here.

(In reply to comment #2) > A few comments: > > * Can you break this into distinct patches for the different pieces here? This > patch is very large and hard to review. I will do it. > > * You don't include ImageFrameSink.*, so no one can actually compile or use > this. ImageFrameSink.* is in bug 27511, still being under review > > * "#if PLATFORM (WINCE)" and similar all over, especially with "#else <existing > code>", really sucks for readability. Making this worse is the fact that > frequently the two blocks of code are very similar or even share many common > parts, and differ only slightly. A combination of helper functions and > subclasses seems like it could do this much more readably. Just don't want to affect other platforms :) > > * goto? Really? It doesn't look necessary at all (a helper function plus a > "continue;" would have served just as well). > > Overall it seems like these capabilities should be added to the existing code > in a way that blends more seamlessly and is usable by any port. If I can > manage to get back to working on Image system cleanup, maybe we can find a way > of doing that stepwise. I don't except that this patch can be merged soon. Agree with that more cleanup is necessary to finally merge it up. Just hope that people can see it, and discuss on this topic, and finally dig out a way how to make it more common. Thanks for your quick response. -Yong

Created attachment 33292 [details] part 1

Created attachment 33293 [details] part 2 GIF Decoder

Created attachment 33294 [details] part 3 JPEG decoder

Created attachment 33296 [details] part 4 PNG decoder

OK... that's not at all what I meant about breaking things into pieces. Those patches are physically separate, but logically intertwined. I meant to break things into patches where each patch added exactly one capability. I think doing that would help address all the concerns above, e.g. make it easier to see how other ports would be affected, and make it easier to test if something decreased execution speed. Overall I'm not worried about decreasing speed by cleaning things up; for example, replacing the gotos with calls to a small non-virtual helper function in the same file will not hurt.

(In reply to comment #9) > OK... that's not at all what I meant about breaking things into pieces. Those > patches are physically separate, but logically intertwined. I meant to break > things into patches where each patch added exactly one capability. > > I think doing that would help address all the concerns above, e.g. make it > easier to see how other ports would be affected, and make it easier to test if > something decreased execution speed. > OK, I will sort it out. > Overall I'm not worried about decreasing speed by cleaning things up; for > example, replacing the gotos with calls to a small non-virtual helper function > in the same file will not hurt. If ImageFrameSink and "segmented" SharedBuffer are accepted for all platforms, then the code can be easier to simplify. I hate those #if ..., too. Without "segmented" SharedBuffer, a 4MB image file must be stored in a 4MB flat memory block as encoded image data. Also, the buffer will be resized many times when ResourceHandle keeps adding new data.

(In reply to comment #10) > If ImageFrameSink and "segmented" SharedBuffer are accepted for all platforms, > then the code can be easier to simplify. I hate those #if ..., too. > > Without "segmented" SharedBuffer, a 4MB image file must be stored in a 4MB flat > memory block as encoded image data. Also, the buffer will be resized many times > when ResourceHandle keeps adding new data. While those two items probably don't affect desktop users too much (I would be surprised if the actual overhead for the buffer resizes is noticeable on the desktop, since we should do few of them), it would definitely be nice for both mobile and desktop use to find a way of trimming the image decoder memory footprint. See for example bug 27308, where I propose re-encoding "large" decoded formats as smaller ones internally, in hopes of reducing memory cost.

Created attachment 33341 [details] make image decoders use ImageFrameSink 1) use ImageFrameSink 2) use secondary GIF reader for size & frame count only 3) able to decode big image down to smaller destination buffer on the fly.

Created attachment 33342 [details] decode from stream source data Make decoders able to decode from stream or segmented source . Decoders can grab segment by segment from the source.

(In reply to comment #13) > Created an attachment (id=33342) [details] > decode from stream source data It looks like this patch expects the prior patch to already be applied. Is that true? Also it looks like you don't modify BMPImageDecoder, ICOImageDecoder, or XBMImageDecoder. Will that cause problems when you encounter one of these image types?

(In reply to comment #14) > (In reply to comment #13) > > Created an attachment (id=33342) [details] [details] > > decode from stream source data > > It looks like this patch expects the prior patch to already be applied. Is > that true? yes. > > Also it looks like you don't modify BMPImageDecoder, ICOImageDecoder, or > XBMImageDecoder. Will that cause problems when you encounter one of these > image types? We haven't supported bmp, ico and xbm images yet. Code blocks under USE(IMAGEFRAMESINK) must be modified to work for other platforms. I just found that there are still something there WINCE-specific. For example: 1) always use 16bit for JPEG images 2) use 16bit for those PNG images that don't have alpha channel 3) decode GIF to 32bit first, checking transparent color during decoding. if the transparent color is unique and it doesn't conflict with other colors after converting 16bit, then we convert the image buffer from 32bit to 16bit. On Windows platform, TransparentBlt can paint an image without specified transparent color used as a mask, which could be cheaper than AlphaBlend. On other platforms, people may want different behaviors.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > Created an attachment (id=33342) [details] [details] [details] > > > decode from stream source data > > > > It looks like this patch expects the prior patch to already be applied. Is > > that true? > > yes. > > > > > Also it looks like you don't modify BMPImageDecoder, ICOImageDecoder, or > > XBMImageDecoder. Will that cause problems when you encounter one of these > > image types? > > We haven't supported bmp, ico and xbm images yet. > > Code blocks under USE(IMAGEFRAMESINK) must be modified to work for other > platforms. > > I just found that there are still something there WINCE-specific. For example: > > 1) always use 16bit for JPEG images > 2) use 16bit for those PNG images that don't have alpha channel > 3) decode GIF to 32bit first, checking transparent color during decoding. if > the transparent color is unique and it doesn't conflict with other colors after > converting 16bit, then we convert the image buffer from 32bit to 16bit. On > Windows platform, TransparentBlt can paint an image without specified > transparent color used as a mask, which could be cheaper than AlphaBlend. > > On other platforms, people may want different behaviors. Correction: "without specified transparent color" should be "with ..."

Comment on attachment 33342 [details] decode from stream source data No ChangeLog, r-. Normaly this would be in the header: + unsigned currentBufferSize() const { return m_currentBufferSize; } no c-style casts: + if (!querySize && m_reader.images_decoded >= (int)haltFrame) while please: + for (;!m_jobComplete;) { Why? +#if PLATFORM(WINCE) && PLATFORM(TORCHMOBILE) + m_secondaryReader->decode(*m_data, GIFFrameCountQuery); +#else m_secondaryReader->decode(m_data.get(), GIFFrameCountQuery); +#endif Peter Kasting should see and comment on this patch please.

Comment on attachment 33341 [details] make image decoders use ImageFrameSink Tabs: 84 for (int scaledX = 0;;) { 85 int x = scaledX * zoom + 0.5; 86 if (x < width) { 87 m_scaledColumns.append(x); 88 ++scaledX; 89 } else 90 break; 91 } Why not while(true) there? No: #if USE(IMAGEFRAMESINK) 251 #else Peter Kasting should see this patch. r- for tabs.

(In reply to comment #17) > (From update of attachment 33342 [details]) > No ChangeLog, r-. > > Normaly this would be in the header: > + unsigned currentBufferSize() const { return m_currentBufferSize; } The class is defined in the cpp.

(In reply to comment #18) > (From update of attachment 33341 [details]) > Tabs: > 84 for (int scaledX = 0;;) { > 85 int x = scaledX * zoom + 0.5; > 86 if (x < width) { > 87 m_scaledColumns.append(x); > 88 ++scaledX; > 89 } else > 90 break; > 91 } > Why not while(true) there? why "while(true)"?

(In reply to comment #20) > (In reply to comment #18) > > (From update of attachment 33341 [details] [details]) > > Tabs: > > 84 for (int scaledX = 0;;) { > > 85 int x = scaledX * zoom + 0.5; > > 86 if (x < width) { > > 87 m_scaledColumns.append(x); > > 88 ++scaledX; > > 89 } else > > 90 break; > > 91 } > > Why not while(true) there? > > why "while(true)"? Definitely not while(true). you initialize a variable for use in the for() and then let it pop off the stack once the loop exits. The code is right as-is.

(In reply to comment #21) > Definitely not while(true). you initialize a variable for use in the for() and > then let it pop off the stack once the loop exits. The code is right as-is. FWIW (which isn't much since I'm not a reviewer), I'm fine with the code the way it's written, but I don't think changing it to expose scaledX and scaledY at the function scope, and using while (true), would hurt the code. This function is pretty short and simple. Dunno if there's an applicable WebKit style rule.

Created attachment 34772 [details] 1) ImageFrameSink seems it should be post here. not except it can be landed soon. but discussions wanted.

Created attachment 34782 [details] 2) use ImageFrameSink in decoders

Created attachment 34783 [details] 3) able to directly write to scaled buffer

Created attachment 34784 [details] 4) able to read from segmented buffer or data stream

Comment on attachment 34783 [details] 3) able to directly write to scaled buffer This is being reworked according to conversation I had with Yong.

Comment on attachment 34783 [details] 3) able to directly write to scaled buffer Just clear the review flag then.

Created attachment 34856 [details] 2) Use ImageFrameSink for JPEG, PNG, and GIF

(In reply to comment #23) > Created an attachment (id=34772) [details] > 1) ImageFrameSink Here's a spattering of comments all over this patch, based on a quick glance. I assume that your motive for resurrecting the "decoded height" variable is so that when you paint a SharedBitmap you can do an opaque blit of the valid height (and avoid the undecoded portion entirely) instead of doing a transparent blit of the entire frame size? If so, I'm not necessarily opposed to resurrecting the height member, although no other ports do this and I wonder if it actually saves much time (it's only relevant while an image is half-decoded and being painted, and at that point the user isn't getting high-speed animation or similar anyway). If it doesn't actually buy meaningful performance, then returning true for frameHasAlphaAtIndex() (either using the method it has now, which I think is fine, or pushing this into all the decoders, which I think is unnecessary code duplication for no benefit) seems like a simpler and better way. I don't understand what createInstance() and deleteInstance() buy you. Callers could just use "foo = new ImageFrameSink(bool);" or "delete foo;". Some of the variable names seem problematic to me; e.g. I would change m_expectedHeight to m_height, m_height to m_decodedHeight, m_bmp to m_bitmap, can to canFreeBuffer. I'm skeptical of the utility of m_compositedWithPreviousFrame. The only use for this would be in decoding GIFs, to avoid creating the complete frame at a particular index. But this means you can't use the "large GIF, discard unnecessary frames" memory optimization which makes an enormous difference for large animated GIFs, because you need to keep around an arbitrary number of frames. Why are the data members protected instead of private? Per Darin Adler we should make things as private as possible. Having both setRGB16() and setRGBA() seems like a poor API for callers. Why doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit? If the answer is "we're worried about putting an if() in there for performance reasons", I'd like to see data on the effect of such a thing before prematurely optimizing. Note that there's already an if() in setRGBA(). Not a big fan of things like "foo >> 1 << 1". "foo & ~0x1" would be clearer. Same with "foo >> 3 << 7"; "(foo << 4) & 0xFFFFFF80" seems clearer. Why does getFrame() return 0 when the status is frameEmpty? An empty frame is still a valid one. The proper use of setCanFreeBuffer() is not obvious to me. Overall there seems to be a lot of functions to explicitly allocate and free things inside the ImageFrameSink. This doesn't feel like a particularly friendly API. Note that the existing RGBA32Buffer has only a few functions, mostly to support deep versus shallow copies of the underlying data. This file is also entirely missing a large number of functions from the current RGBA32Buffer, such as clear(), copyRowNTimes(), asNewNativeImage(), etc., which are used in various different cases. I assume this is because you wrote this code quite a while ago. If we wanted to use ImageFrameSink, I think a lot of these would need to be supported, so you wouldn't be massively rewriting the decoders or other cross-platform Image code (especially if such rewrites removed optimizations, like the GIF frame-clearing optimization).

(In reply to comment #29) > Created an attachment (id=34856) [details] > 2) Use ImageFrameSink for JPEG, PNG, and GIF I don't see the point of marking r? on this when it can't land until ImageFrameSink does, and that's a ways away. Much of this patch is a copy-and-paste of old code from the image decoders that has since been changed for both clarity and correctness. You need to discard that and find a way to modify in-line the existing decoding code, instead of completely rewriting entire functions under #ifdefs. We still wouldn't land the patch that way, but it would at least allow a precise understanding of what ImageFrameSink requires, which is impossible with this patch.

(In reply to comment #30) > (In reply to comment #23) > > Created an attachment (id=34772) [details] [details] > > 1) ImageFrameSink > > Here's a spattering of comments all over this patch, based on a quick glance. > > I assume that your motive for resurrecting the "decoded height" variable is so > that when you paint a SharedBitmap you can do an opaque blit of the valid > height (and avoid the undecoded portion entirely) instead of doing a > transparent blit of the entire frame size? If so, I'm not necessarily opposed > to resurrecting the height member, although no other ports do this and I wonder > if it actually saves much time (it's only relevant while an image is > half-decoded and being painted, and at that point the user isn't getting > high-speed animation or similar anyway). If it doesn't actually buy meaningful > performance, then returning true for frameHasAlphaAtIndex() (either using the > method it has now, which I think is fine, or pushing this into all the > decoders, which I think is unnecessary code duplication for no benefit) seems > like a simpler and better way. The performance issue is very valid for our target platform.

(In reply to comment #32) > (In reply to comment #30) > > I wonder > > if it actually saves much time (it's only relevant while an image is > > half-decoded and being painted, and at that point the user isn't getting > > high-speed animation or similar anyway). If it doesn't actually buy meaningful > > performance, then returning true for frameHasAlphaAtIndex() (either using the > > method it has now, which I think is fine, or pushing this into all the > > decoders, which I think is unnecessary code duplication for no benefit) seems > > like a simpler and better way. > > The performance issue is very valid for our target platform. OK, do you have some data (e.g. page load times or animation frame rates) around this issue? It would be helpful in deciding whether we should also try to convert the other backends over to a similar mechanism. I could be totally overlooking a particular case where this is really useful.

(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #30) > > > I wonder > > > if it actually saves much time (it's only relevant while an image is > > > half-decoded and being painted, and at that point the user isn't getting > > > high-speed animation or similar anyway). If it doesn't actually buy meaningful > > > performance, then returning true for frameHasAlphaAtIndex() (either using the > > > method it has now, which I think is fine, or pushing this into all the > > > decoders, which I think is unnecessary code duplication for no benefit) seems > > > like a simpler and better way. > > > > The performance issue is very valid for our target platform. > > OK, do you have some data (e.g. page load times or animation frame rates) > around this issue? It would be helpful in deciding whether we should also try > to convert the other backends over to a similar mechanism. I could be totally > overlooking a particular case where this is really useful. No numbers right now but alpha blending is ugly, blitting is slow. Every pixel causes pain here. I don't expect it to be an issue in other places.

(In reply to comment #30) > (In reply to comment #23) > > Created an attachment (id=34772) [details] [details] > > 1) ImageFrameSink > > Here's a spattering of comments all over this patch, based on a quick glance. > > I assume that your motive for resurrecting the "decoded height" variable is so > that when you paint a SharedBitmap you can do an opaque blit of the valid > height (and avoid the undecoded portion entirely) instead of doing a > transparent blit of the entire frame size? If so, I'm not necessarily opposed > to resurrecting the height member, although no other ports do this and I wonder > if it actually saves much time (it's only relevant while an image is > half-decoded and being painted, and at that point the user isn't getting > high-speed animation or similar anyway). If it doesn't actually buy meaningful > performance, then returning true for frameHasAlphaAtIndex() (either using the > method it has now, which I think is fine, or pushing this into all the > decoders, which I think is unnecessary code duplication for no benefit) seems > like a simpler and better way. > Seems RGBA32Buffer will be modified to support more formats. Are you planning to do this? After that, you will find that JPEG doesn't have alpha channel, so it may use 24bit format or RGB565 to store decoded data. So even you hack frameHasAlphaAtIndex(), it won't work. Also, on some devices, alpha blending may be slow, or even not supported.

(In reply to comment #35) > Seems RGBA32Buffer will be modified to support more formats. Are you planning > to do this? I'm planning to remove the name "RGBA32" from the class, since that class actually doesn't support any formats at all; the underlying native image types do. Though right now none of those native image types uses anything other than 32-bit ARGB (or RGBA, or whatever), they could, and my suspicion is that that decision is best left to those classes. I haven't coded it so I can't be sure, of course, but I think rather than having things like is16Bit() and raw bit-writing ops directly in this header, those should be pushed to places like what you've written as SharedBuffer. Being able to cleanly integrate ports like WinCE that desire lower-bitdepth storage formats is a key motivator to doing this, so whatever solution we land on certainly has to work for you guys. > After that, you will find that JPEG doesn't have alpha channel, so > it may use 24bit format or RGB565 to store decoded data. It could; it's not clear without trying it whether that will be a big win for backends like Skia and Cairo. Backends that did this would also need to ensure that their code doesn't rely on the image storage being 32-bit; I think Skia might, at the moment. > So even you hack frameHasAlphaAtIndex(), it won't work. FWIW, at the moment Cairo uses frameHasAlphaAtIndex() to determine its compositing op, but Skia just uses the real image data inside the SkBitmap, and ignores this function entirely. It might make sense to have that kind of pattern everywhere, since I imagine most native image storage formats can tell you if they use an alpha channel... > Also, on some devices, alpha blending may be slow, or even not supported. Having it not supported entirely would make a platform pretty unusable since not only most of the image formats, but all kinds of other CSS, SVG, etc. operations need alpha. So I'm not as concerned with designing for that case. Slow is another story; we do probably want to make it possible for ports to avoid alpha-blending where possible. I wonder if we need ensureHeight() and friends to do that, though; in the current RGBA32Buffer API, it would be pretty simple to set the decoded height correctly on an underlying native image without needing an explicit call from the decoder. This is because the decoders move monotonically through the rows of an image, and set the status to FrameComplete when they finish.

(In reply to comment #30) > (In reply to comment #23) > > Created an attachment (id=34772) [details] [details] > > 1) ImageFrameSink > > I don't understand what createInstance() and deleteInstance() buy you. Callers > could just use "foo = new ImageFrameSink(bool);" or "delete foo;". > createInstance is useful in this case: class ImageFrameSinkXxxxWithMoreMembers : public ImageFrameSink { more members here; }; ImageFrameSink* ImageFrameSink::createInstance() { return new ImageFrameSinkXxxxWithMoreMembers ; } > > Some of the variable names seem problematic to me; e.g. I would change > m_expectedHeight to m_height, m_height to m_decodedHeight m_height as RGBA32Buffer defined. Yeah, now it's ok to change the name as m_height has been removed from RGBA32Buffer. > > I'm skeptical of the utility of m_compositedWithPreviousFrame. The only use > for this would be in decoding GIFs, to avoid creating the complete frame at a > particular index. But this means you can't use the "large GIF, discard > unnecessary frames" memory optimization which makes an enormous difference for > large animated GIFs, because you need to keep around an arbitrary number of > frames. m_compositedWithPreviousFrame is not only for "avoiding someting", but also important for transparent effects in GIF animation. > > Why are the data members protected instead of private? Per Darin Adler we > should make things as private as possible. because the subclasses need to access them > > Having both setRGB16() and setRGBA() seems like a poor API for callers. Why > doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit? > If the answer is "we're worried about putting an if() in there for performance > reasons", I'd like to see data on the effect of such a thing before prematurely > optimizing. Note that there's already an if() in setRGBA(). they also have different inputs > > Not a big fan of things like "foo >> 1 << 1". "foo & ~0x1" would be clearer. >> 1 << 1 seems cleaner to. but I'm not sure which one is faster. > Same with "foo >> 3 << 7"; "(foo << 4) & 0xFFFFFF80" seems clearer. Same thing here, I don't think the latter is clean. > > Why does getFrame() return 0 when the status is frameEmpty? An empty frame is > still a valid one. why? Who likes to waste CPU time on empty frame? > > The proper use of setCanFreeBuffer() is not obvious to me. This nofities the buffer that decoder has finished writting to the buffer, so the buffer can be released when it's under memory pressure. > > Overall there seems to be a lot of functions to explicitly allocate and free > things inside the ImageFrameSink. This doesn't feel like a particularly > friendly API. Note that the existing RGBA32Buffer has only a few functions, > mostly to support deep versus shallow copies of the underlying data. > > This file is also entirely missing a large number of functions from the current > RGBA32Buffer, such as clear(), copyRowNTimes(), asNewNativeImage(), etc., which > are used in various different cases. I assume this is because you wrote this > code quite a while ago. If we wanted to use ImageFrameSink, I think a lot of > these would need to be supported, so you wouldn't be massively rewriting the > decoders or other cross-platform Image code (especially if such rewrites > removed optimizations, like the GIF frame-clearing optimization). I still think it's not difficult to replace RGBA32Buffer with ImageFrameSink. Yeah, it needs some work definitely. The one that I'm most concerned is about GIF m_compositedWithPreviousFrame thing. That's complicated, and ImageFrameSink code works in a little different way. Want to talk to somebody on this.

(In reply to comment #37) > (In reply to comment #30) > > I don't understand what createInstance() and deleteInstance() buy you. Callers > > could just use "foo = new ImageFrameSink(bool);" or "delete foo;". > > > createInstance is useful in this case: > > class ImageFrameSinkXxxxWithMoreMembers : public ImageFrameSink > { > more members here; > }; > > ImageFrameSink* ImageFrameSink::createInstance() > { > return new ImageFrameSinkXxxxWithMoreMembers ; > } But does anyone do that? It doesn't look like it from this patch. Why build factory functions unless you need them? > > I'm skeptical of the utility of m_compositedWithPreviousFrame. The only use > > for this would be in decoding GIFs, to avoid creating the complete frame at a > > particular index. But this means you can't use the "large GIF, discard > > unnecessary frames" memory optimization which makes an enormous difference for > > large animated GIFs, because you need to keep around an arbitrary number of > > frames. > > m_compositedWithPreviousFrame is not only for "avoiding someting", but also > important for transparent effects in GIF animation. I don't understand. I know how GIF animation and frame disposal methods work. The existing ports handle them correctly without using this mechanism. Why doesn't the existing mechanism work for you, and how do you deal with not having the frame-clearing optimization for large GIFs? I would think on a memory-constrained platform that'd be even more important than it is on desktops. > > Why are the data members protected instead of private? Per Darin Adler we > > should make things as private as possible. > > because the subclasses need to access them What subclasses? > > Having both setRGB16() and setRGBA() seems like a poor API for callers. Why > > doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit? > > they also have different inputs This response is vague enough that I haven't learned anything. What is the difference and why can't it be created at the SharedBuffer or ImageFrameSink level? > > Why does getFrame() return 0 when the status is frameEmpty? An empty frame is > > still a valid one. > > why? Who likes to waste CPU time on empty frame? What CPU is being wasted? We don't blit anything with empty frames. > > The proper use of setCanFreeBuffer() is not obvious to me. > > This nofities the buffer that decoder has finished writting to the buffer, so > the buffer can be released when it's under memory pressure. Isn't this the same signal as setting the status to FrameComplete? > The one that I'm most concerned is about GIF m_compositedWithPreviousFrame > thing. That's complicated, and ImageFrameSink code works in a little different > way. Want to talk to somebody on this. Maybe you could write up a short doc with descriptions of the existing code and your code, pointing out the differences and why they are valuable? I can guess how someone would implement GIF animation with a flag like this but I can't see how to do it without needing to keep a potentially arbitrary number of frames around, for example if you scroll an animated GIF out of the viewport and then back in (causing the animation to need to spin forward).

(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #30) > > But does anyone do that? It doesn't look like it from this patch. Why build > factory functions unless you need them? See first patch. We have ImageFrameSinkWince defined which holds a SharedBitmap as the buffer provider. > > I don't understand. I know how GIF animation and frame disposal methods work. > The existing ports handle them correctly without using this mechanism. Why > doesn't the existing mechanism work for you, and how do you deal with not > having the frame-clearing optimization for large GIFs? I would think on a > memory-constrained platform that'd be even more important than it is on > desktops. I've fixed many bugs on it, and our browser runs OK. Let me review this and get back to you. I did that so long time ago :) > What subclasses? ImageFrameSinkWince, if I am not wrong. > > > > Having both setRGB16() and setRGBA() seems like a poor API for callers. Why > > > doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit? > > > > they also have different inputs > > This response is vague enough that I haven't learned anything. What is the > difference and why can't it be created at the SharedBuffer or ImageFrameSink > level? I'm lost now. I thought you mean why setRGB16 and setRGBA cannot share a same function. If that was the question, I would say separate functions are definitely better. > > > > Why does getFrame() return 0 when the status is frameEmpty? An empty frame is > > > still a valid one. > > > > why? Who likes to waste CPU time on empty frame? > > What CPU is being wasted? We don't blit anything with empty frames. So why cannot it return 0? > > > > The proper use of setCanFreeBuffer() is not obvious to me. > > > > This nofities the buffer that decoder has finished writting to the buffer, so > > the buffer can be released when it's under memory pressure. > > Isn't this the same signal as setting the status to FrameComplete? FrameComplete is just for a single frame. But GIF decoder may still want to read a finished frame when it processes the next one. > > > The one that I'm most concerned is about GIF m_compositedWithPreviousFrame > > thing. That's complicated, and ImageFrameSink code works in a little different > > way. Want to talk to somebody on this. > > Maybe you could write up a short doc with descriptions of the existing code and > your code, pointing out the differences and why they are valuable? ok. but for why they are valuable, I have mentioned in bug 26467, see comment #5

(In reply to comment #39) > (In reply to comment #38) > > (In reply to comment #37) > > > (In reply to comment #30) > > > > But does anyone do that? It doesn't look like it from this patch. Why build > > factory functions unless you need them? > > See first patch. We have ImageFrameSinkWince defined which holds a SharedBitmap > as the buffer provider. I looked at most of the patches and missed any ImageFrameSinkWince class. It looks in this patch like you have an ImageFrameSinkWince.cpp, but it's just filling out the functions of ImageFrameSink and doesn't do any subclassing. Maybe I'm missing something. > > I don't understand. I know how GIF animation and frame disposal methods work. > > The existing ports handle them correctly without using this mechanism. Why > > doesn't the existing mechanism work for you, and how do you deal with not > > having the frame-clearing optimization for large GIFs? I would think on a > > memory-constrained platform that'd be even more important than it is on > > desktops. > > I've fixed many bugs on it, and our browser runs OK. Let me review this and get > back to you. I did that so long time ago :) Try cases like http://www.aintitcool.com/files/HoD2.gif and http://img2.abload.de/img/almost_failedovs.gif and make sure that: * Images animate in the same time as Chrome 2/Safari 4 (not slower) * You don't blow out memory or CPU usage > > > > Having both setRGB16() and setRGBA() seems like a poor API for callers. Why > > > > doesn't setRGBA() just write the data in 16-bit format if the bitmap is 16-bit? > > > > > > they also have different inputs > > > > This response is vague enough that I haven't learned anything. What is the > > difference and why can't it be created at the SharedBuffer or ImageFrameSink > > level? > > I'm lost now. I thought you mean why setRGB16 and setRGBA cannot share a same > function. If that was the question, I would say separate functions are > definitely better. It sounds like we're not understanding each other. My object is to reduce the complexity and special-casing of shared code, especially the decoders themselves. Let's say platform X supports native image storage in various formats (565, 888, 8888, etc.). The pattern that makes sense to me is: Image Decoder reads the pixel values out of the source image and calls setRGBA with them ImageFrameSink/RGBA32Buffer/whatever passes these values along to the backend's underlying NativeImage functions These functions decide how to downsample to an appropriate format With your existing code, the downsampling would need to be done in the decoder, which would need to check whether the image frame claims to be 16-bit. All that seems to do is cause us to plumb more state up through shared code and push the complexity on everyone. > > > > Why does getFrame() return 0 when the status is frameEmpty? An empty frame is > > > > still a valid one. > > > > > > why? Who likes to waste CPU time on empty frame? > > > > What CPU is being wasted? We don't blit anything with empty frames. > > So why cannot it return 0? Practically, it can; there are only two reasons I wouldn't do it: * The API is lying * A backend can't read any data off a NULL pointer, such as the size of the frame, if it wants it If it makes no difference, I don't see why we should lie to callers. It's not a big deal, it just seems like a waste to explicitly check for an empty frame so we can lie. > > > > The proper use of setCanFreeBuffer() is not obvious to me. > > > > > > This nofities the buffer that decoder has finished writting to the buffer, so > > > the buffer can be released when it's under memory pressure. > > > > Isn't this the same signal as setting the status to FrameComplete? > > FrameComplete is just for a single frame. But GIF decoder may still want to > read a finished frame when it processes the next one. So it's _not_ about the decoder being finished writing to the buffer. It's some signal that the decoder is finished reading the buffer too. In that case it seems like a bug that you don't finishBitmap() when we go to FrameComplete. In this case I suggest copying clear() from the existing code, which is designed for precisely this functionality. Note that there are fewer member variables involved (no "should" and "can" distinction). > > > The one that I'm most concerned is about GIF m_compositedWithPreviousFrame > > > thing. That's complicated, and ImageFrameSink code works in a little different > > > way. Want to talk to somebody on this. > > > > Maybe you could write up a short doc with descriptions of the existing code and > > your code, pointing out the differences and why they are valuable? > > ok. but for why they are valuable, I have mentioned in bug 26467, see comment > #5 It's not at all obvious in the code that these are useful for dynamic image scaling. Nor do I see what they buy you for scaling versus the decoding technique in the existing code. I think I'd benefit from a writeup :)

(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > > (In reply to comment #37) > > > > (In reply to comment #30) > > > > I looked at most of the patches and missed any ImageFrameSinkWince class. It > looks in this patch like you have an ImageFrameSinkWince.cpp, but it's just > filling out the functions of ImageFrameSink and doesn't do any subclassing. > > Maybe I'm missing something. Hm... Sorry, ImageFrameSinkWince has been removed by refactoring... too long story. But createInstance and deleteInstance are already added, providing ability to subclass ImageFrameSink, which should be fine. > > > > I don't understand. I know how GIF animation and frame disposal methods work. > > > The existing ports handle them correctly without using this mechanism. Why > > > doesn't the existing mechanism work for you, and how do you deal with not > > > having the frame-clearing optimization for large GIFs? I would think on a > > > memory-constrained platform that'd be even more important than it is on > > > desktops. > > > > I've fixed many bugs on it, and our browser runs OK. Let me review this and get > > back to you. I did that so long time ago :) > > Try cases like http://www.aintitcool.com/files/HoD2.gif and > http://img2.abload.de/img/almost_failedovs.gif and make sure that: > * Images animate in the same time as Chrome 2/Safari 4 (not slower) > * You don't blow out memory or CPU usage Thanks for providing this test case. I'm confident our browser won't crash :) Will try it > > My object is to reduce the complexity and special-casing of shared code, > especially the decoders themselves. Let's say platform X supports native image > storage in various formats (565, 888, 8888, etc.). The pattern that makes > sense to me is: > > Image Decoder reads the pixel values out of the source image and calls setRGBA > with them > ImageFrameSink/RGBA32Buffer/whatever passes these values along to the backend's > underlying NativeImage functions > These functions decide how to downsample to an appropriate format > > With your existing code, the downsampling would need to be done in the decoder, > which would need to check whether the image frame claims to be 16-bit. All > that seems to do is cause us to plumb more state up through shared code and > push the complexity on everyone. I think you make a good point here. > > > > > > Why does getFrame() return 0 when the status is frameEmpty? An empty frame is > > > > > still a valid one. > > > > > > > > why? Who likes to waste CPU time on empty frame? > > > > > > What CPU is being wasted? We don't blit anything with empty frames. > > > > So why cannot it return 0? > > Practically, it can; there are only two reasons I wouldn't do it: > * The API is lying > * A backend can't read any data off a NULL pointer, such as the size of the > frame, if it wants it > > If it makes no difference, I don't see why we should lie to callers. It's not > a big deal, it just seems like a waste to explicitly check for an empty frame > so we can lie. The object itself has size and all information. getFrame() just return a platform image, which can be null when it doesn't want to return a valid image. for example, failed to allocated memory for it. > > > > > > The proper use of setCanFreeBuffer() is not obvious to me. > > > > > > > > This nofities the buffer that decoder has finished writting to the buffer, so > > > > the buffer can be released when it's under memory pressure. > > > > > > Isn't this the same signal as setting the status to FrameComplete? > > > > FrameComplete is just for a single frame. But GIF decoder may still want to > > read a finished frame when it processes the next one. > > So it's _not_ about the decoder being finished writing to the buffer. It's > some signal that the decoder is finished reading the buffer too. In that case > it seems like a bug that you don't finishBitmap() when we go to FrameComplete. finisheBitmap() is called when ImageSource is taking the bitmap.

(In reply to comment #41) > > My object is to reduce the complexity and special-casing of shared code, > > especially the decoders themselves. Let's say platform X supports native image > > storage in various formats (565, 888, 8888, etc.). The pattern that makes > > sense to me is: > > > > Image Decoder reads the pixel values out of the source image and calls setRGBA > > with them > > ImageFrameSink/RGBA32Buffer/whatever passes these values along to the backend's > > underlying NativeImage functions > > These functions decide how to downsample to an appropriate format > > > > With your existing code, the downsampling would need to be done in the decoder, > > which would need to check whether the image frame claims to be 16-bit. All > > that seems to do is cause us to plumb more state up through shared code and > > push the complexity on everyone. > > I think you make a good point here. You both are assuming that the decoder can stream pixels directly into the native image buffer. That simply isn't the case for some of the existing users of the built-in image decoders. This was covered I think by Holger. It might not even be the case that the native image buffer is in main memory. At this point, I think the discussion should move away from WinCE's ImageFrameSink and more towards the path we want to take in the future. I still do not believe we are all on the same page. Perhaps a conference call or two with all interested parties would be in order.

> Try cases like http://www.aintitcool.com/files/HoD2.gif and > http://img2.abload.de/img/almost_failedovs.gif and make sure that: > * Images animate in the same time as Chrome 2/Safari 4 (not slower) > * You don't blow out memory or CPU usage Hm... you're right, these are too big GIF, we have to clean the memory for previous frames. thanks.

(In reply to comment #43) > > Try cases like http://www.aintitcool.com/files/HoD2.gif and > > http://img2.abload.de/img/almost_failedovs.gif and make sure that: > > * Images animate in the same time as Chrome 2/Safari 4 (not slower) > > * You don't blow out memory or CPU usage > > Hm... you're right, these are too big GIF, we have to clean the memory for > previous frames. thanks. seems the crash is due to some changes made in upstream. our previous builds work with them with no problem. investigating

Comment on attachment 34772 [details] 1) ImageFrameSink Clearing flags as these need more discussion anyways