Bug 230630 – [WPE] Add gamepad support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Víctor M. Jáquez L.]

Add gamepad support through libwpe.

[Víctor M. Jáquez L.]

Created attachment 439033 [details]
Patch

[Víctor M. Jáquez L.]

Created attachment 439064 [details]
Patch

[Carlos Alberto Lopez Perez]

Patch failed to build on the WPE EWS

Is because it needs https://github.com/WebPlatformForEmbedded/libwpe/pull/88 into libwpe?

[Víctor M. Jáquez L.]

(In reply to Carlos Alberto Lopez Perez from comment #3)
> Patch failed to build on the WPE EWS
> 
> Is because it needs https://github.com/WebPlatformForEmbedded/libwpe/pull/88
> into libwpe?

Yes, that API is required.

[Zan Dobersek]

Comment on attachment 439064 [details]
Patch

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

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:467
> +#if ENABLE(GAMEPAD)
> +WebKit::WebPageProxy* View::platformWebPageProxyForGamepadInput()
> +{
> +    const auto& views = viewsVector();
> +    if (views.isEmpty())
> +        return nullptr;
> +
> +    auto index = views.findMatching([](View* view) {
> +        return view->viewState().contains(WebCore::ActivityState::IsVisible)
> +            && view->viewState().contains(WebCore::ActivityState::IsFocused);
> +    });
> +
> +    if (index != notFound)
> +        return &(views[index]->page());
> +
> +    return nullptr;
> +}
> +#endif

This is not ideal. The wpe_gamepad API should be handled so that the embedding application has the control over what view the provider is assigned to, and covering the possibility of that provider being assigned to a different view during its lifetime.

[Víctor M. Jáquez L.]

(In reply to Zan Dobersek from comment #5)
> Comment on attachment 439064 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439064&action=review
> 
> > Source/WebKit/UIProcess/API/wpe/WPEView.cpp:467
> > +#if ENABLE(GAMEPAD)
> > +WebKit::WebPageProxy* View::platformWebPageProxyForGamepadInput()
> > +{
> > +    const auto& views = viewsVector();
> > +    if (views.isEmpty())
> > +        return nullptr;
> > +
> > +    auto index = views.findMatching([](View* view) {
> > +        return view->viewState().contains(WebCore::ActivityState::IsVisible)
> > +            && view->viewState().contains(WebCore::ActivityState::IsFocused);
> > +    });
> > +
> > +    if (index != notFound)
> > +        return &(views[index]->page());
> > +
> > +    return nullptr;
> > +}
> > +#endif
> 
> This is not ideal. The wpe_gamepad API should be handled so that the
> embedding application has the control over what view the provider is
> assigned to, and covering the possibility of that provider being assigned to
> a different view during its lifetime.

Right.

[Víctor M. Jáquez L.]

Created attachment 451516 [details]
Patch

[Víctor M. Jáquez L.]

 (In reply to Zan Dobersek from comment #5)
>
> This is not ideal. The wpe_gamepad API should be handled so that the
> embedding application has the control over what view the provider is
> assigned to, and covering the possibility of that provider being assigned to
> a different view during its lifetime.

I guess this request is now addressed. So, gently ping :)

Remind this patch needs: https://github.com/WebPlatformForEmbedded/libwpe/pull/88

And, to test it in cog: https://github.com/Igalia/cog/pull/361

[Adrian Perez]

Comment on attachment 451516 [details]
Patch

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

Other than a few suggestions here and there, this is looking quite good to me.
Probably we can merge it after a round of polish :-)

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:48
> +    static struct wpe_gamepad_client_interface s_client = {

This can be “static const”.

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:51
> +            auto& self = *reinterpret_cast<WPEGamepad*>(data);

Use static_cast<WPEGamepad*> here.

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:57
> +            auto& self = *reinterpret_cast<WPEGamepad*>(data);

Same here, use static_cast<WPEGamepad*>.

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:66
> +        value.setValue(0.0);

There is a Vector(value, size) constructor that can be used initialize this
automatically during construction, without needing an explicit resize+loop.
See note below.

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:76
> +        return;

I would actually ASSERT(!m_gamepad) because the pointer is always
initialized in the constructor, so unless something weird happens the
pointer should always be valid during the lifetime of the WPEGamepad
instance.

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.h:51
> +    Vector<SharedGamepadValue> m_buttonValues;

Given that GAMEPAD_BUTTON_COUNT is a constant, this could be a
Vector<SharedGamepadValue, GAMEPAD_BUTTON_COUNT> to store the
values inline inside the WPEGamepad. As a side effect that avoids
a heap allocation and has better cache locality (less pointer chasing).

Also you can do the initialization here:

    Vector<SharedGamepadValue, GAMEPAD_BUTTON_COUNT> m_buttonValues { GAMEPAD_BUTTON_COUNT, 0.0 };

> Source/WebCore/platform/gamepad/wpe/WPEGamepad.h:54
> +    struct wpe_gamepad* m_gamepad { nullptr };

How about using a std::unique_ptr<struct wpe_gamepad, wpe_gamepad_destroy> here?

> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:57
> +    static struct wpe_gamepad_provider_client_interface s_client = {

This can be “static const”.

> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:60
> +            auto& provider = *reinterpret_cast<WPEGamepadProvider*>(data);

static_cast

> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:65
> +            auto& provider = *reinterpret_cast<WPEGamepadProvider*>(data);

static_cast

> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:155
> +void WPEGamepadProvider::gamepadDiconnected(unsigned id)

Typo: gamepadDiconnected -> gamepadDisconnected

> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:54
> +}

I'm not much of a fan of adding global data, but I would need to think a
bit to try to come up with some alternative. Right not I don't have anything
from the top of my head, and this definitely works, so I won't ask to change
it 😉️

[Víctor M. Jáquez L.]

Comment on attachment 451516 [details]
Patch

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

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:48
>> +    static struct wpe_gamepad_client_interface s_client = {
> 
> This can be “static const”.

it cannot be const because the function's signature for wpe_gamepad_set_client isn't either.

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:51
>> +            auto& self = *reinterpret_cast<WPEGamepad*>(data);
> 
> Use static_cast<WPEGamepad*> here.

done

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:57
>> +            auto& self = *reinterpret_cast<WPEGamepad*>(data);
> 
> Same here, use static_cast<WPEGamepad*>.

done

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:66
>> +        value.setValue(0.0);
> 
> There is a Vector(value, size) constructor that can be used initialize this
> automatically during construction, without needing an explicit resize+loop.
> See note below.

I didn't initialized it in the header but in the class constructor, because libwpe.h enums aren't exposed in the header file.

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.cpp:76
>> +        return;
> 
> I would actually ASSERT(!m_gamepad) because the pointer is always
> initialized in the constructor, so unless something weird happens the
> pointer should always be valid during the lifetime of the WPEGamepad
> instance.

added the ASSERT

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.h:51
>> +    Vector<SharedGamepadValue> m_buttonValues;
> 
> Given that GAMEPAD_BUTTON_COUNT is a constant, this could be a
> Vector<SharedGamepadValue, GAMEPAD_BUTTON_COUNT> to store the
> values inline inside the WPEGamepad. As a side effect that avoids
> a heap allocation and has better cache locality (less pointer chasing).
> 
> Also you can do the initialization here:
> 
>     Vector<SharedGamepadValue, GAMEPAD_BUTTON_COUNT> m_buttonValues { GAMEPAD_BUTTON_COUNT, 0.0 };

I used the initializator but not in this way, because of the libwpe.h header inclusion.

>> Source/WebCore/platform/gamepad/wpe/WPEGamepad.h:54
>> +    struct wpe_gamepad* m_gamepad { nullptr };
> 
> How about using a std::unique_ptr<struct wpe_gamepad, wpe_gamepad_destroy> here?

done

Also for the wpe_gamepad_provider. Though, didn't include the function there, but in the initializator in class consturctor.

>> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:57
>> +    static struct wpe_gamepad_provider_client_interface s_client = {
> 
> This can be “static const”.

As  in wpe_gamepad, it cannot be const because of the wpe's function signature.

>> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:60
>> +            auto& provider = *reinterpret_cast<WPEGamepadProvider*>(data);
> 
> static_cast

done

>> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:65
>> +            auto& provider = *reinterpret_cast<WPEGamepadProvider*>(data);
> 
> static_cast

done

>> Source/WebCore/platform/gamepad/wpe/WPEGamepadProvider.cpp:155
>> +void WPEGamepadProvider::gamepadDiconnected(unsigned id)
> 
> Typo: gamepadDiconnected -> gamepadDisconnected

done! hehehe

>> Source/WebKit/UIProcess/API/wpe/WPEView.cpp:54
>> +}
> 
> I'm not much of a fan of adding global data, but I would need to think a
> bit to try to come up with some alternative. Right not I don't have anything
> from the top of my head, and this definitely works, so I won't ask to change
> it 😉️

ok :)

[Víctor M. Jáquez L.]

Created attachment 458817 [details]
Patch

[Víctor M. Jáquez L.]

We need to talk how and when merge this because of the version bumps needed for cog and libwpe.

[Víctor M. Jáquez L.]

(In reply to Víctor M. Jáquez L. from comment #12)
> We need to talk how and when merge this because of the version bumps needed
> for cog and libwpe.

There's an interesting situation here: WebXR WPE implementation also has its own gamepad implementation, through OpenXR, but it excludes the html gamepad. It's specific to WebXR.

It brings some questions:

1\ Doesn't both implementations step one over the other?
2\ If user disable gamepad, it's enabled for WebXR silently
3\ There's not a implicit feature mechanism to disable one but enabling the other.

[Víctor M. Jáquez L.]

Created attachment 459107 [details]
Patch

[Víctor M. Jáquez L.]

Pull request: https://github.com/WebKit/WebKit/pull/911

[EWS]

Committed 253720@main (ab0e24752e11): <https://commits.webkit.org/253720@main>

Reviewed commits have been landed. Closing PR #911 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/99078410>