Bug 30304 – struct RGBA32Buffer is twice as big as the old ImageData of Qt

• Component: Platform
• Status: RESOLVED INVALID

[Holger Freyther]

The RGBA32Buffer takes more space than required and more space than the old ImageDecoderQt::ImageData. The proposed change is to change the object layout and move properties only useful for the GIF decoder into the gif decoder. This change reduces the size of the struct from 52 byte to 20 byte with a byte spare for future use.

The setRect method of RGBA32Buffer is only useful for animations (currently only used for GIF). There is no use to store the IntSize of the image and then again in the IntRect (taking 16bytes). Move the IntRect into into the GIF decoder and remove setRect from the other decoders.

The two enum values for FrameStatus and FrameDispoal take four byte each, with the current size of the enum it should not take more than four bits in total. Propose to change it to uint8_t with a COMPILE_ASSERT to verify that the enum stays below 256 values.

[Holger Freyther]

Created attachment 41032 [details]
Reduce RGBA32Buffer from 52bytes to 20 bytes per instance

This is the first version of the patch.

[Peter Kasting]

I haven't done more than a ten-second scan of the patch, but the idea sounds good to me.  I'm more pleased with the "scoping" effect of putting the rects into the GIF decoder than the real memory savings -- seems like 32 bytes * at most hundreds of images on a page is going to be a few K of savings overall, which isn't really noticeable.  OTOH, it never hurts to make things more compact as long as it doesn't add obfuscation.

[Eric Seidel (no email)]

Comment on attachment 41032 [details]
Reduce RGBA32Buffer from 52bytes to 20 bytes per instance

I've never seen WebKit use this style before:
 IntRect& rect = m_rects[frameIndex];
 272     rect = frameRect;

I think we should either abstract that into a function call where it is OK to pass a &, or we should use explicit m_rects[frameIndex] = calls.  Or maybe it would be OK wiht a better variable name than "rect".

I really think pkasting is your best reviewer, even if he's not technically a reviewer yet.

[Peter Kasting]

Yeah, non-const refs used as lvalues throughout a function is a bit subtle.  Either using explicit "m_rects[frameIndex] = ..." or using a pointer instead of a ref would probably be fine with me (the first might be more explicitly obvious and therefore better?).

[Darin Adler]

(In reply to comment #3)
> (From update of attachment 41032 [details])
> I've never seen WebKit use this style before:
>  IntRect& rect = m_rects[frameIndex];
>  272     rect = frameRect;

I've used that sort of idiom in code I wrote for WebKit. I’ll try to find it.

[Peter Kasting]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 41032 [details] [details])
> > I've never seen WebKit use this style before:
> >  IntRect& rect = m_rects[frameIndex];
> >  272     rect = frameRect;
> 
> I've used that sort of idiom in code I wrote for WebKit. I’ll try to find it.

Even if there is precedent, I think other ways of doing this are better by virtue of being less subtle.

[Holger Freyther]

The precedent is in the same file. One line above..

@@ -393,6 +398,7 @@ void GIFImageDecoder::frameComplete(unsigned frameIndex, unsigned frameDuration,
     // Initialize the frame if necessary.  Some GIFs insert do-nothing frames,
     // in which case we never reach haveDecodedRow() before getting here.
     RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
+    IntRect& rect = m_rects[frameIndex];

I'm a bit puzzled by the double standards. I'm going to change the occurence GIFImageDecoder::initFrameBuffer though as a pointer is used for the RGBA32Buffer.

[Peter Kasting]

(In reply to comment #7)
>      RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];

I fully support changing this too!  Never even saw it.

(That said, there is one difference -- in this function, members of |buffer| get set and called, but |buffer| is never used directly as an lvalue, whereas assigning to a ref looks more like an assignment to a temp.  But I think this distinction is pretty trivial, and doesn't justify one case over the other.)

[Holger Freyther]

Created attachment 41472 [details]
Reduce RGBA32Buffer from 52bytes to 20 bytes per instance (take two)

Updated patch:

When RGBA32Buffer& frame was used IntRect& frame will be used
When RGBA32Buffer* frame was used IntRect* frame will be used.


m_rects[frameIndex] = newRect is specially not used as the frameIndex method parameter is changed in the function body and will point to a different "frame" when the assignment would be due.

[Holger Freyther]

(In reply to comment #8)
> (In reply to comment #7)
> >      RGBA32Buffer& buffer = m_frameBufferCache[frameIndex];
> 
> I fully support changing this too!  Never even saw it.
> 
> (That said, there is one difference -- in this function, members of |buffer|
> get set and called, but |buffer| is never used directly as an lvalue, whereas
> assigning to a ref looks more like an assignment to a temp.  But I think this
> distinction is pretty trivial, and doesn't justify one case over the other.)

How would you change it? Going for pointers?

RGA32Buffer* buffer = &m_frameBuffer..
buffer->setFoo(foo);

IntRect* rect = &m_Rects
rect->operator=(*newRect);
*rect = *newRect?

or make frameIndex const in the method body and use m_rects[frameIndex] = newRect?

[Peter Kasting]

(In reply to comment #10)
> How would you change it? Going for pointers?
> 
> RGA32Buffer* buffer = &m_frameBuffer..
> buffer->setFoo(foo);

Sure, that'd be fine

> IntRect* rect = &m_Rects
> rect->operator=(*newRect);
> *rect = *newRect?

"*rect = *newRect", please, not "rect->operator=(*newRect)"

> or make frameIndex const in the method body and use m_rects[frameIndex] =
> newRect?

This is good too

[Holger Freyther]

Created attachment 42028 [details]
Follow up patch to move from RGBA32Buffer& frame to RGBA32Buffer* frame and the same for the IntRect

Change the code to use a RGBA32Buffer* and an IntRect* instead of the reference as discussed here.

[Darin Adler]

Comment on attachment 42028 [details]
Follow up patch to move from RGBA32Buffer& frame to RGBA32Buffer* frame and the same for the IntRect

> +    RGBA32Buffer* const buffer = &m_frameBufferCache[frameIndex];
> +    IntRect* const rect = &m_rects[frameIndex];

It seems a little strange to use const here, even though references can't be rebound and hence are const-like. We don't normally mark local variables const even though the vast majority of them are indeed not modified from their initial values and could be marked const if we thought there was significant benefit.

[Peter Kasting]

(In reply to comment #13)
> We don't normally mark local variables const
> even though the vast majority of them are indeed not modified from their
> initial values and could be marked const if we thought there was significant
> benefit.

I ran into this because I tend to mark everything const that could be.  The comments I was given were that while this was unusual in WebCore code, it was OK, and so a lot of the Image code I've touched uses const on locals.

[Darin Adler]

(In reply to comment #14)
> I ran into this because I tend to mark everything const that could be.

I like to do that on my own personal programming projects too, but I don't do it in WebKit.

> The
> comments I was given were that while this was unusual in WebCore code, it was
> OK, and so a lot of the Image code I've touched uses const on locals.

Your use of the passive voice here obscures an important question. Who made those comments?

[Peter Kasting]

(In reply to comment #15)
> (In reply to comment #14)
> > The
> > comments I was given were that while this was unusual in WebCore code, it was
> > OK, and so a lot of the Image code I've touched uses const on locals.
> 
> Your use of the passive voice here obscures an important question. Who made
> those comments?

I don't recall.  eseidel perhaps?  Could have been somebody else.

If we want to discourage this, it seems like we should go ahead and add it to the style guide.

[Darin Adler]

(In reply to comment #16)
> If we want to discourage this, it seems like we should go ahead and add it to
> the style guide.

If you’d like to start doing this in WebKit, I think you should talk with other people about it in webkit-dev. It’s not debatable what the current style is. Look at existing WebKit code. Just open almost any source file such as Frame.cpp and you'll see lots of cases where const could be used for a local variable and is not.

I don’t think it’s the burden of the style guide to list every convention that’s followed. In general, the burden is on the person who wants to change style to tell other people about it on the mailing list and get some feedback before simply doing it in new code with one reviewer looking at it.

On the other hand, I’d welcome a comment mentioning in this in the style guide. And I also think it might be good to start using const for local variables. It’s a bit verbose to have all those extra const, but also neat to be able to distinguish variables that don’t change from those that do.

[Peter Kasting]

(In reply to comment #17)
> If you’d like to start doing this in WebKit, I think you should talk with other
> people about it in webkit-dev. It’s not debatable what the current style is.

I guess I don't feel like const-on-locals is something that needs to be consistent between files.  For example, there are certainly many places where we use "Foo*" where "const Foo*" would be appropriate, but I've never heard a complaint of inconsistent style for the people who do use "const Foo*".  We're not consistent about when we use ?: versus if (), preincrement versus postincrement, etc.  How do we decide whether this is significant enough that we should be consistent with other files, versus something trivial enough to change?

(I note that using a non-const ref is something that's similarly not mentioned anywhere, but I got picky about it on this bug.  I might be being hypocritical?)

> And I also think it might be good to start using const for local variables.

I would like that in principle, but it seems over-burdensome to effectively deprecate much of the codebase by saying that everywhere should move consistently to using const where possible.  (Although I suppose we've done something similarly broad with the namespace-indenting-in-headers change.)  I guess this is really another way of saying the same thing I said in the above paragraph.

[Darin Adler]

(In reply to comment #18)
> I guess I don't feel like const-on-locals is something that needs to be
> consistent between files.

OK. We disagree.

[Darin Adler]

(In reply to comment #18)
> (I note that using a non-const ref is something that's similarly not mentioned
> anywhere, but I got picky about it on this bug. I might be being
> hypocritical?)

I don’t agree with your non-const ref preference. To me code with references reads just fine.

[Holger Freyther]

I just tried to make  GIFImageDecoder::initFrame and GIFImageDecoder::frameComplete consistent in regard to ptr vs. reference as I got negative review making the IntRect a reference in ::frameComple.

I will remove const from all four pointers and hope that the actual patch gets a review.

[Eric Seidel (no email)]

Comment on attachment 41472 [details]
Reduce RGBA32Buffer from 52bytes to 20 bytes per instance (take two)

I think  170         uint8_t m_disposalMethod;
can just be a FrameDisposalMethod
the need to use unsigned, etc, comes from bitfields, or from the fact that some compilers default enums to being signed.  But here it doesn't seem to be an issue.  BUt if you're rathe rleave it as uint for consistence, I guess that's fine too.

Not to jump back into the previous discussion, but I'm confused why this isn't const:
 401     IntRect& rect = m_rects[frameIndex];

rect.contains() is a const method, no?

I think you meant 8 for both of these:
36 COMPILE_ASSERT(RGBA32Buffer::FrameStatusLast < 256, FrameStatusFitsIn8Byte);
 37 COMPILE_ASSERT(RGBA32Buffer::DisposeLast < 256, DisposeLastFitsIn9Byte);

This seems fine though.

[Eric Seidel (no email)]

Comment on attachment 42028 [details]
Follow up patch to move from RGBA32Buffer& frame to RGBA32Buffer* frame and the same for the IntRect

No opinion here.  I'll let Darin comment.

[Darin Adler]

(In reply to comment #23)
> No opinion here.  I'll let Darin comment.

What am I supposed to on? I think I already did comment.

[Eric Seidel (no email)]

(In reply to comment #24)
> (In reply to comment #23)
> > No opinion here.  I'll let Darin comment.
> 
> What am I supposed to on? I think I already did comment.

Apologies, I thought the attachment was new, post the const discussion.

[Eric Seidel (no email)]

Comment on attachment 42028 [details]
Follow up patch to move from RGBA32Buffer& frame to RGBA32Buffer* frame and the same for the IntRect

I don't see why rect is non-const to begin with.  If I were writing this code, I would have written:

const IntRect& rect = m_rects[frameIndex];
in the first place.

this change has been aroudn a while and no one has r+'d it, so I guess I'll express an opinion and mark it r- as unecessary.  Sorry that you seem to have stepped into a style land-mine on this bug.  I'm not going to argue with someone else if they chose to r+ this patch.

[Eric Seidel (no email)]

Attachment 41472 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.

[Holger Freyther]

Comment on attachment 41472 [details]
Reduce RGBA32Buffer from 52bytes to 20 bytes per instance (take two)

Clearing review flag as the Qt part needs another twist due change in Qt.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.