Bug 225673 – Factor copyImagePixel pixel conversion code into its own file

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Factor copyImagePixel pixel conversion code into its own file

[Sam Weinig]

Created attachment 428315 [details]
Patch

[Darin Adler]

Comment on attachment 428315 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428315&action=review

> Source/WebCore/platform/graphics/PixelBufferConversion.h:44
> +void convertImagePixels(const PixelBufferConversionView& source, PixelBufferConversionView& destination, const IntSize&);

Not so clear that destination should be non-const here. I understand that the pixels are overwritten, but is that really best expressed by passing a non-const reference to the view? In every other way, the view is an in parameter, not an out parameter. And seeing it passed as a reference doesn’t really make that clear to me. It would be a huge programming error to expect the function to set fields like destination.alphaFormat.

I’m also not entirely clear why IntSize is not part of the conversion view; perhaps because it’s needed only for source and not destination? Or because it must be the same for both?

Is there a better way to express this function? Any small adjustment that could make semantics a little more obvious? Maybe it would be as simple as taking the data pointer out of the view and perhaps it would have a different name then? That certainly would make the source worse, but then you could see how the source and destination const-ness differed.

Should be able to use const uint8_t* for the source pixels.

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 428315 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428315&action=review
> 
> > Source/WebCore/platform/graphics/PixelBufferConversion.h:44
> > +void convertImagePixels(const PixelBufferConversionView& source, PixelBufferConversionView& destination, const IntSize&);
> 
> Not so clear that destination should be non-const here. I understand that
> the pixels are overwritten, but is that really best expressed by passing a
> non-const reference to the view? In every other way, the view is an in
> parameter, not an out parameter. And seeing it passed as a reference doesn’t
> really make that clear to me. It would be a huge programming error to expect
> the function to set fields like destination.alphaFormat.
> 
> I’m also not entirely clear why IntSize is not part of the conversion view;
> perhaps because it’s needed only for source and not destination? Or because
> it must be the same for both?
> 
> Is there a better way to express this function? Any small adjustment that
> could make semantics a little more obvious? Maybe it would be as simple as
> taking the data pointer out of the view and perhaps it would have a
> different name then? That certainly would make the source worse, but then
> you could see how the source and destination const-ness differed.
> 
> Should be able to use const uint8_t* for the source pixels.

In another project I work on, I added some similar code a little while back and the way I handled this was by having two classes:

class ConstPixelBufferView;
class PixelBufferView;

Where ConstPixelBufferView had a `const uint8_t*` (actually, I used `const std::byte*) and PixelBufferView had a `uint8_t*` (or rather `std::byte*`). They implemented mostly the same, using a template base class, and that worked quite nicely. My plan was to start small here and work up to that. I want these views to eventually have a nice interface for iterating pixels and getting inset buffers.

Do you think a ConstPixelBufferConversionView for the source, and PixelBufferConversionView for the destination would make things better here?

[Darin Adler]

(In reply to Sam Weinig from comment #3)
> Do you think a ConstPixelBufferConversionView for the source, and
> PixelBufferConversionView for the destination would make things better here?

Yes. And then both would be const& parameters.

There may be some even better pattern, but if you had that pattern here I would have never have made any comment!

[Sam Weinig]

Created attachment 428335 [details]
Patch

[Darin Adler]

Comment on attachment 428335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428335&action=review

> Source/WebCore/platform/graphics/PixelBufferConversion.h:50
> +struct PixelBufferConversionView {
> +    AlphaPremultiplication alphaFormat;
> +    DestinationColorSpace colorSpace;
> +    PixelFormat pixelFormat;
> +    unsigned bytesPerRow;
> +    uint8_t* rows;
> +};
> +
> +struct ConstPixelBufferConversionView {
> +    AlphaPremultiplication alphaFormat;
> +    DestinationColorSpace colorSpace;
> +    PixelFormat pixelFormat;
> +    unsigned bytesPerRow;
> +    const uint8_t* rows;
> +};

To make this less repetitive, we could make a structure:

    struct PixelBufferFormat { };

Then we could either derive the two views from it, or have a data member of that type. It would have everything except the rows pointer.

[EWS]

Committed r277369 (237627@main): <https://commits.webkit.org/237627@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428335 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77908051>

[Darin Adler]

Comment on attachment 428335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=428335&action=review

> Source/WebCore/platform/graphics/PixelBufferConversion.cpp:42
> +template<typename View> static inline vImage_Buffer makeVImageBuffer(const View& view, const IntSize& size)

Looking over this file, I see inline a lot. And I realize that we probably haven’t discussed inline in the WebKit project enough. Here is my take on it:

Using the inline keyword to guide optimization (literal "inlining"):
- The inline keyword itself doesn’t guarantee inlining, nor is it typically required to make inlining possible. In the modern compilers, just having the function definition visible in the translation unit is most of what is needed to make inlining possible without a global optimization pass.
- If we find that the compiler is not inlining and we want that to improve execution speed or code size, adding the inline keyword might help, but we may find that we need the ALWAYS_INLINE keyword instead.

Using the inline keyword for correctness:
- We need to use the inline keyword when we put a non-template function into a header so that the one-definition rule is relaxed so we can include it in multiple translation units. This also making inlining possible without global optimization, as mentioned above.
- Also note that the static keyword does not need to be used in this case; using it to ask for internal linkage may be actively harmful, and affects things like the linkage of global variables defined inside the function.
- The above rules do *not* apply for functions that is in a non-header, defined inside a class definition, a function template, or marked constexpr. In all those cases we don’t need inline for correctness.

Given these considerations, if I was the programmer, I would have landed this patch without any of the inline keywords in this source file.

[Sam Weinig]

(In reply to Darin Adler from comment #9)
> Comment on attachment 428335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428335&action=review
> 
> > Source/WebCore/platform/graphics/PixelBufferConversion.cpp:42
> > +template<typename View> static inline vImage_Buffer makeVImageBuffer(const View& view, const IntSize& size)
> 
> Looking over this file, I see inline a lot. And I realize that we probably
> haven’t discussed inline in the WebKit project enough. Here is my take on it:
> 
> Using the inline keyword to guide optimization (literal "inlining"):
> - The inline keyword itself doesn’t guarantee inlining, nor is it typically
> required to make inlining possible. In the modern compilers, just having the
> function definition visible in the translation unit is most of what is
> needed to make inlining possible without a global optimization pass.
> - If we find that the compiler is not inlining and we want that to improve
> execution speed or code size, adding the inline keyword might help, but we
> may find that we need the ALWAYS_INLINE keyword instead.
> 
> Using the inline keyword for correctness:
> - We need to use the inline keyword when we put a non-template function into
> a header so that the one-definition rule is relaxed so we can include it in
> multiple translation units. This also making inlining possible without
> global optimization, as mentioned above.
> - Also note that the static keyword does not need to be used in this case;
> using it to ask for internal linkage may be actively harmful, and affects
> things like the linkage of global variables defined inside the function.
> - The above rules do *not* apply for functions that is in a non-header,
> defined inside a class definition, a function template, or marked constexpr.
> In all those cases we don’t need inline for correctness.
> 
> Given these considerations, if I was the programmer, I would have landed
> this patch without any of the inline keywords in this source file.

That seems reasonable, though I always assumed compilers used the inline keyword as a hint, though I have no basis to back that up.

These seem like good rules. I copied the inline from the previous location, and didn't think about it much. I'll remove them.

[Fujii Hironori]

Filed: Bug 225725 – [WinCairo] Some webgl/2.0.0/conformance/textures/image_bitmap_from_image_data are failing after r277369