[Webkit-unassigned] [Bug 27511] Add WinCE specific platform/graphics files to WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27511





--- Comment #59 from Peter Kasting <pkasting at google.com>  2009-08-08 22:27:11 PDT ---
(In reply to comment #58)
> > > +struct jpeg_dest_mgr {
> > > +    struct jpeg_destination_mgr pub;
> > > +    Vector<char> buffer;
> > > +    Vector<char>* dump;
> > > +};
> > 
> > Naming style (here and below)
> 
> I see { should be in the next line here. I'm told that "m_" should not be used
> for "struct"

Based on http://webkit.org/coding/coding-style.html , the struct name should be
CamelCase (with the first letter capitalized), and the members should have m_
(I don't see an exception for structs and haven't heard this before).  Also the
names should be more descriptive than "dest_mgr" or "pub"; use full words where
possible.

> > Also, the 2k capacity value seems arbitrary.  How do we know that that's a good
> > value?  Why not just:
> >     Vector<JSAMPLE> rowBuffer(compressData.image_width * 3);
> > ?
> try to benefit from inline vector. if total bytes of a row is less than 2k, we
> don't need to allocate memory from heap.

I haven't used this technique with the WTF library so I'm pretty ignorant of
its implementation detail.  If you don't declare the 2k capacity, is the vector
guaranteed to heap-allocate no matter the size, or would it still do something
smart for you under the hood?  How was 2k picked here -- was it arbitrary, or
based on some data?  Is there a downside other than stack usage?  Comments
would be helpful.

> > > +            for (const unsigned short* rowEnd = pixel + compressData.image_width; pixel < rowEnd;) {
> > > +                unsigned short value = *pixel++;
> > 
> > Might be a little clearer to do:
> > for (...; ...; ++pixel) {
> >     unsigned short value = *pixel;
> 
> *foo++ is very popular in image processing code. Not sure if it can help
> compiler optimization.

I cannot speak with perfect authority, but I did spend five years as a compiler
engineer, and my impression is that your code is if anything less likely to be
optimized than my suggestion.  (It's very likely the two generate the same
code.)

It would be awesome if you had some sort of testcase that exercised a lot of
this code in a way that could be timed.  Of course benchmarking inner loops is
tricky given the effects of processor caches and the like, but such a thing
would be immensely valuable in the future, where I believe we'll try to
rearchitect the Image system to include the capabilities you need more
directly, instead of as something of a fork (like you've had to maintain until
now).  It would help reassure me that, for example, replacing some particular
pixel-writing loop with a call to an inlined function did not actually hurt
performance.  Otherwise, the best we can do is probably manual assembly
inspection which is pretty error-prone when determining real perf.

> > > +        for (; pixel < pixelEnd; ) {
> > 
> > I find "while (pixel < pixelEnd) {" clearer.
> 
> Seems webkit prefer "while" to "for". Somehow I started hating "while" a few
> years ago.

I think I'm a bigger fan of for() (over while()) than most, since I tend to get
nitpicky about minimizing variable scopes.  But in the case where there is no
index variable declared and no iteration clause, while() means precisely what
you're doing, whereas for() just results in adding ; ; and making things more
opaque.  Precision in syntax is a virtue...

Anyway, glad you found the comments helpful.  I haven't reviewed the other
patches, other things might arise there.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.