Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GLIB] Generate serialization for DMABuf classes #28725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

psaavedra
Copy link
Contributor

@psaavedra psaavedra commented May 17, 2024

4b24153

[GLIB] Generate serialization for DMABuf classes
https://bugs.webkit.org/show_bug.cgi?id=274328

Reviewed by NOBODY (OOPS!).

Explanation of why this fixes the bug (OOPS!).
* Added new serialization description files for `DMABufColorSpace`,
  `DMABufFormat`, `DMABufObject` and `DMABufReleaseFlag`.
* Removed custom encode/decode methods in `DMABufObject`.
* Fixed argument coder templates in `Encoder.h` and `Decoder.h` by
  removing unnecessary `void` parameters.
* Updated `CMakeLists.txt` in WebCore and WebKit to include DMABuf
  serialization files.

* Source/WebCore/CMakeLists.txt:
* Source/WebCore/platform/graphics/gbm/DMABufColorSpace.h:
* Source/WebCore/platform/graphics/gbm/DMABufColorSpace.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufFormat.h:
(WebCore::DMABufFormat::Plane::Plane):
* Source/WebCore/platform/graphics/gbm/DMABufFormat.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufObject.h:
(WebCore::DMABufObject::DMABufObject): Deleted.
(WebCore::DMABufObject::encode): Deleted.
(WebCore::DMABufObject::decode): Deleted.
* Source/WebCore/platform/graphics/gbm/DMABufObject.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:
(WebCore::DMABufReleaseFlag::DMABufReleaseFlag):
* Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.serialization.in: Added.
* Source/WebKit/CMakeLists.txt:
* Source/WebKit/Platform/IPC/Decoder.h:
(IPC::Decoder::decode):
* Source/WebKit/Platform/IPC/Encoder.h:

4b24153

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style ❌ πŸ›  ios ❌ πŸ›  mac βœ… πŸ›  wpe ❌ πŸ›  wincairo
βœ… πŸ§ͺ bindings ❌ πŸ›  ios-sim ❌ πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2 ❌ πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl ❌ πŸ§ͺ ios-wk2 ❌ πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
❌ πŸ§ͺ ios-wk2-wpt ❌ πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-cairo
❌ πŸ§ͺ api-ios ❌ πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
❌ πŸ›  tv ❌ πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
❌ πŸ›  tv-sim ❌ πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
❌ πŸ›  watch
❌ πŸ›  watch-sim

@psaavedra psaavedra requested a review from cdumez as a code owner May 17, 2024 19:40
@psaavedra psaavedra self-assigned this May 17, 2024
@psaavedra psaavedra added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 17, 2024
@psaavedra
Copy link
Contributor Author

Motivation:

A recent commit (https://commits.webkit.org/278057@main) breaks the WPEWebKit build when GPU_PROCESS=ON with this error in Source/WebKit/Platform/IPC/Encoder.h:

/sources/wpewebkit/Source/WebKit/Platform/IPC/Encoder.h:80:60: error: incomplete type 'IPC::ArgumentCoder<WebCore::DMABufObject, void>' used in nested name specifier
   80 |         ArgumentCoder<std::remove_cvref_t<T>, void>::encode(*this, std::forward<T>(t));
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

@psaavedra psaavedra force-pushed the eng/GLIB-Generate-serialization-for-DMABuf-classes branch from 9903d05 to eb7f9f6 Compare May 17, 2024 19:57
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 17, 2024
Copy link
Contributor

@aperezdc aperezdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch looks nice overall, I only have a couple of doubts but those might be because I have not touched much the new autogenerated serialization stuff. That's why it would be better someone else checks this as wellβ€”maybe @csaavedra, as he has been doing serialization patches lately?

@@ -1,6 +1,6 @@
/*
* Copyright (C) 2022 Metrological Group B.V.
* Copyright (C) 2022 Igalia S.L.
* Copyright (C) 2024 Igalia S.L.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not technically needed to update copyright years but when we do, typically we use ranges:

Suggested change
* Copyright (C) 2024 Igalia S.L.
* Copyright (C) 2022-2024 Igalia S.L.

@@ -126,6 +126,12 @@ struct DMABufFormat {
, verticalSubsampling(PlaneDefinitionType::verticalSubsampling)
{ }

Plane(const FourCC& fourcc, const unsigned& hsValue, const unsigned& vsValue)
Copy link
Contributor

@aperezdc aperezdc May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor used during deserialization could be made private by adding a friend statement like this:

friend struct IPC::ArgumentCoder<Plane, void>;

as part of the struct Plane declaration.

@@ -40,20 +40,6 @@
namespace WebCore {

struct DMABufObject {
WTF_MAKE_NONCOPYABLE(DMABufObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to remove WTF_MAKE_NONCOPYABLE() here? As far as I tried, the autogenerated serialization code can deal with non-copyable objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the generated code uses move semantics, so if this is getting copied somewhere, that should be fixed instead.

std::optional<T> t { ArgumentCoder<std::remove_cvref_t<T>, void>::decode(*this) };
std::optional<T> t { ArgumentCoder<std::remove_cvref_t<T>>::decode(*this) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this is needed. While working on #27313 one of the WIP versions of the patch added a new .serialization.in and this did not need to be changed πŸ€”

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature of the encode methods autogenerated change a bit:

In file included from sources/wpewebkit/Source/WebKit/Shared/WebPreferencesStore.h:29,
                 from sources/wpewebkit/Source/WebKit/UIProcess/WebPreferences.h:31,
                 from wpewebkit-2.45.upstream-20240511/DerivedSources/WebKit/WebPreferencesFeatures.cpp:29:
sources/wpewebkit/Source/WebKit/Platform/IPC/Encoder.h: In member function 'IPC::Encoder& IPC::Encoder::operator<<(T&&)':
sources/wpewebkit/Source/WebKit/Platform/IPC/Encoder.h:80:46: error: wrong number of template arguments (2, should be 1)
   80 |         ArgumentCoder<std::remove_cvref_t<T, void>>::encode(*this, std::forward<T>(t));
      |                                              ^~~~
In file included from recipe-sysroot/usr/include/c++/13.2.0/bits/stl_pair.h:60,
                 from recipe-sysroot/usr/include/c++/13.2.0/bits/stl_algobase.h:64,
                 from recipe-sysroot/usr/include/c++/13.2.0/algorithm:60,
                 from sources/wpewebkit/Source/WebKit/WebKit2Prefix.h:60,
                 from <command-line>:
recipe-sysroot/usr/include/c++/13.2.0/type_traits:3442:11: note: provided for 'template<class _Tp> using std::remove_cvref_t = typename std::remove_cvref::type'
 3442 |     using remove_cvref_t = typename remove_cvref<_Tp>::type;
      |           ^~~~~~~~~~~~~~
sources/wpewebkit/Source/WebKit/Platform/IPC/Encoder.h:80:50: error: template argument 1 is invalid
   80 |         ArgumentCoder<std::remove_cvref_t<T, void>>::encode(*this, std::forward<T>(t));
      |                                                  ^~
sources/wpewebkit/Source/WebKit/Platform/IPC/Encoder.h:80:86: error: expression list treated as compound expression in initializer [-fpermissive]
   80 |         ArgumentCoder<std::remove_cvref_t<T, void>>::encode(*this, std::forward<T>(t));
      |                                                                                      ^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However it seems this is an issue for the Apple ports:

In file included from /Volumes/Data/worker/macOS-Ventura-Release-Build-EWS/build/WebKitBuild/Release/DerivedSources/TestWebKitAPI/unified-sources/UnifiedSource40-mm.mm:3:
In file included from /Volumes/Data/worker/macOS-Ventura-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:29:
/Volumes/Data/worker/macOS-Ventura-Release-Build-EWS/build/Source/WebKit/Platform/IPC/Encoder.h:80:9: error: too few template arguments for class template 'ArgumentCoder'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should do this change.

@aperezdc aperezdc requested review from csaavedra and a team May 17, 2024 20:36
@psaavedra psaavedra removed the merging-blocked Applied to prevent a change from being merged label May 17, 2024
@psaavedra psaavedra force-pushed the eng/GLIB-Generate-serialization-for-DMABuf-classes branch from eb7f9f6 to 75ae8d9 Compare May 17, 2024 20:37
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 17, 2024
https://bugs.webkit.org/show_bug.cgi?id=274328

Reviewed by NOBODY (OOPS!).

Explanation of why this fixes the bug (OOPS!).
* Added new serialization description files for `DMABufColorSpace`,
  `DMABufFormat`, `DMABufObject` and `DMABufReleaseFlag`.
* Removed custom encode/decode methods in `DMABufObject`.
* Fixed argument coder templates in `Encoder.h` and `Decoder.h` by
  removing unnecessary `void` parameters.
* Updated `CMakeLists.txt` in WebCore and WebKit to include DMABuf
  serialization files.

* Source/WebCore/CMakeLists.txt:
* Source/WebCore/platform/graphics/gbm/DMABufColorSpace.h:
* Source/WebCore/platform/graphics/gbm/DMABufColorSpace.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufFormat.h:
(WebCore::DMABufFormat::Plane::Plane):
* Source/WebCore/platform/graphics/gbm/DMABufFormat.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufObject.h:
(WebCore::DMABufObject::DMABufObject): Deleted.
(WebCore::DMABufObject::encode): Deleted.
(WebCore::DMABufObject::decode): Deleted.
* Source/WebCore/platform/graphics/gbm/DMABufObject.serialization.in: Added.
* Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.h:
(WebCore::DMABufReleaseFlag::DMABufReleaseFlag):
* Source/WebCore/platform/graphics/gbm/DMABufReleaseFlag.serialization.in: Added.
* Source/WebKit/CMakeLists.txt:
* Source/WebKit/Platform/IPC/Decoder.h:
(IPC::Decoder::decode):
* Source/WebKit/Platform/IPC/Encoder.h:
@psaavedra psaavedra removed the merging-blocked Applied to prevent a change from being merged label May 18, 2024
@psaavedra psaavedra force-pushed the eng/GLIB-Generate-serialization-for-DMABuf-classes branch from 75ae8d9 to 4b24153 Compare May 18, 2024 06:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 18, 2024
Comment on lines +2840 to +2843
platform/graphics/gbm/DMABufColorSpace.serialization.in
platform/graphics/gbm/DMABufFormat.serialization.in
platform/graphics/gbm/DMABufObject.serialization.in
platform/graphics/gbm/DMABufReleaseFlag.serialization.in
Copy link
Member

@csaavedra csaavedra May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These files preferrably should go to WebKit/Shared/glib/ instead of WebCore. Also, you could just put all DMABuf definitions in a single file to avoid clutter (there is already one, merging that one too into a new ArgumentCodersDMABuf.serialization.in).

Copy link
Contributor Author

@psaavedra psaavedra May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also apply to .h files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily. I don't think you need to change those.

# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

header: <WebCore/DMABufColorSpace.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need the header if the class for which a coder is being generated has a header with the exact same name and .h extension. header: is used when there are additional headers that need to be pulled and/or if the header has a different name (in which case you can use [CustomHeader] to prevent the default to be added).

Comment on lines +33 to +36
namespace IPC {
template<typename T, typename U> struct ArgumentCoder;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply include <wtf/ArgumentCoder.h> instead, but I don't think you need this, read below.

Comment on lines +137 to +144
private:
Plane(const FourCC& fourcc, const unsigned& hsValue, const unsigned& vsValue)
: fourcc(fourcc)
, horizontalSubsampling(hsValue)
, verticalSubsampling(vsValue)
{ }

friend struct IPC::ArgumentCoder<Plane, void>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to add an explicit constructor since this is a struct and the data members are public, so aggregate initialization should work. If you use the same order in the serialization declaration as they are declared in the struct, the generated code should be able to create a new instance with something like Plane { WTFMove(arg1), WTFMove(arg2), WTFMove(arg3) }.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, in c++, structs can indeed benefit from aggregate initialization if they meet certain conditions like no user-provided or inherited constructors, no private or protected non-static data members, no virtual functions, and no virtual, private, or protected base classes.

However, the struct Plane does not qualify for aggregate initialization because it includes a user-provided constructor.

    struct Plane {                                                                                                     
        Plane() = default;                                                                                             
    
        template<typename PlaneDefinitionType>                                                                         
        Plane(const PlaneDefinitionType&)                                                                              
            : fourcc(PlaneDefinitionType::fourcc)                                                                      
            , horizontalSubsampling(PlaneDefinitionType::horizontalSubsampling)                                        
            , verticalSubsampling(PlaneDefinitionType::verticalSubsampling)                                            
        { }   
...

Comment on lines +23 to +24
header: <WebCore/DMABufFormat.h>
struct WebCore::DMABufFormat {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above regarding header:.

Comment on lines +25 to +26
header: <WebCore/DMABufObject.h>
[CustomHeader] struct WebCore::DMABufObject {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above regarding header:.

std::optional<T> t { ArgumentCoder<std::remove_cvref_t<T>, void>::decode(*this) };
std::optional<T> t { ArgumentCoder<std::remove_cvref_t<T>>::decode(*this) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should do this change.

ArgumentCoder<std::remove_cvref_t<T>, void>::encode(*this, std::forward<T>(t));
ArgumentCoder<std::remove_cvref_t<T>>::encode(*this, std::forward<T>(t));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should do this change.

Comment on lines +23 to +24
header: <WebCore/DMABufReleaseFlag.h>
struct WebCore::DMABufReleaseFlag {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above regarding header:.

@@ -40,20 +40,6 @@
namespace WebCore {

struct DMABufObject {
WTF_MAKE_NONCOPYABLE(DMABufObject);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the generated code uses move semantics, so if this is getting copied somewhere, that should be fixed instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merging-blocked Applied to prevent a change from being merged WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
5 participants