Bug 216174 – Fix Internals::supportsVCPEncoder on BigSur

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Fix Internals::supportsVCPEncoder on BigSur

[youenn fablet]

<rdar://problem/66492801>

[youenn fablet]

Created attachment 407970 [details]
Patch

[EWS]

Committed r266614: <https://trac.webkit.org/changeset/266614>

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

[Myles C. Maxfield]

Did this patch cause this?

CompileC Internals-227951BA24B9AE61.o
/Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/testing/Internals.cpp:5684:34: error: '&&' within '||' [-Werror,-Wlogical-op-parentheses]
    return ENABLE_VCP_ENCODER || ENABLE_VCP_VTB_ENCODER || HAVE_VTB_REQUIREDLOWLATENCY;
                                 ^~~~~~~~~~~~~~~~~~~~~~ ~~
In file included from /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/testing/Internals.cpp:336:
/Users/mmaxfield/Build/Products/Debug/usr/local/include/webrtc/sdk/WebKit/VideoProcessingSoftLink.h:45:74: note: expanded from macro 'ENABLE_VCP_VTB_ENCODER'
#define ENABLE_VCP_VTB_ENCODER __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500 && __MAC_OS_X_VERSION_MIN_REQUIRED < 110000
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/testing/Internals.cpp:5684:34: note: place parentheses around the '&&' expression to silence this warning
    return ENABLE_VCP_ENCODER || ENABLE_VCP_VTB_ENCODER || HAVE_VTB_REQUIREDLOWLATENCY;
                                 ^~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/mmaxfield/src/WebKit/OpenSource/Source/WebCore/testing/Internals.cpp:336:
/Users/mmaxfield/Build/Products/Debug/usr/local/include/webrtc/sdk/WebKit/VideoProcessingSoftLink.h:45:74: note: expanded from macro 'ENABLE_VCP_VTB_ENCODER'
#define ENABLE_VCP_VTB_ENCODER __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500 && __MAC_OS_X_VERSION_MIN_REQUIRED < 110000
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

[Myles C. Maxfield]

I reverted this patch and now the build is successful.

[Myles C. Maxfield]

Comment on attachment 407970 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:5684
> +    return ENABLE_VCP_ENCODER || ENABLE_VCP_VTB_ENCODER || HAVE_VTB_REQUIREDLOWLATENCY;

Shouldn't this be ENABLE(VCP_ENCODER) || ENABLE(VCP_VTB_ENCODER) || HAVE(VTB_REQUIREDLOWLATENCY)?

[Myles C. Maxfield]

I can commit a short-term fix that modifies Internals.cpp to put parentheses around ENABLE_VCP_VTB_ENCODER, but it seems like the parentheses really should be in Source/ThirdParty/libwebrtc/Source/webrtc/sdk/WebKit/VideoProcessingSoftLink.h. However, I don't know what the process is for updating this third-party library, so it would be good if someone could look into this for the long term.

[youenn fablet]

Please revert the change, I’ll reland a fully valid version.

[Myles C. Maxfield]

Committed r266657: <https://trac.webkit.org/changeset/266657>

[Myles C. Maxfield]

Oh, whoops, sorry, I didn't see your message!

[Myles C. Maxfield]

(In reply to Myles C. Maxfield from comment #6)
> Comment on attachment 407970 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407970&action=review
> 
> > Source/WebCore/testing/Internals.cpp:5684
> > +    return ENABLE_VCP_ENCODER || ENABLE_VCP_VTB_ENCODER || HAVE_VTB_REQUIREDLOWLATENCY;
> 
> Shouldn't this be ENABLE(VCP_ENCODER) || ENABLE(VCP_VTB_ENCODER) ||
> HAVE(VTB_REQUIREDLOWLATENCY)?

#define ENABLE(WTF_FEATURE) (defined ENABLE_##WTF_FEATURE && ENABLE_##WTF_FEATURE)

I think switching to ENABLE(VCP_VTB_ENCODER) would actually have fixed this.

[Ryosuke Niwa]

(In reply to Myles C. Maxfield from comment #9)
> Committed r266657: <https://trac.webkit.org/changeset/266657>

This patch broke mac builds:

https://trac.webkit.org/changeset/266657/webkit

/Volumes/Data/slave/mojave-release/build/Source/WebCore/testing/Internals.cpp:5684:31: error: use of logical '||' with constant operand [-Werror,-Wconstant-logical-operand]
    return ENABLE_VCP_ENCODER || (ENABLE_VCP_VTB_ENCODER) || HAVE_VTB_REQUIREDLOWLATENCY;
                              ^  ~~~~~~~~~~~~~~~~~~~~~~~~
/Volumes/Data/slave/mojave-release/build/Source/WebCore/testing/Internals.cpp:5684:31: note: use '|' for a bitwise operation
    return ENABLE_VCP_ENCODER || (ENABLE_VCP_VTB_ENCODER) || HAVE_VTB_REQUIREDLOWLATENCY;
                              ^~
                              |

[WebKit Commit Bot]

Re-opened since this is blocked by bug 216212

[Ryosuke Niwa]

I reverted both the original patch & Myle's build fix attempt in https://trac.webkit.org/r266658 since it's 3am here and I can't think straight to land a proper build fix as I had only 4 hours of sleep last night.

[youenn fablet]

Created attachment 408176 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[youenn fablet]

Created attachment 408183 [details]
Patch for landing

[EWS]

Committed r266701: <https://trac.webkit.org/changeset/266701>

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

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 408226 [details]
Patch

[Geoffrey Garen]

Comment on attachment 408226 [details]
Patch

r=me

[Geoffrey Garen]

If this doesn't do the trick, let's use some window.internals API to wait for the pixels.

[Geoffrey Garen]

Another thing to consider is to mark this test to run individually instead of in parallel with other tests, since that can reduce system load when running this test.

[EWS]

Committed r266738: <https://trac.webkit.org/changeset/266738>

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