<rdar://problem/66492801>

Created attachment 407970 [details]
Patch

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

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

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.

I reverted this patch and now the build is successful.

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)?

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.

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

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

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

(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.

(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;
                              ^~
                              |

Re-opened since this is blocked by bug 216212

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.

Created attachment 408176 [details]
Patch for landing

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Created attachment 408183 [details]
Patch for landing

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

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

Reopening to attach new patch.

Created attachment 408226 [details]
Patch

Comment on attachment 408226 [details]
Patch

r=me

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

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.

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

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