Bug 202159

Summary: Add ANGLE backend for iOS device
Product: WebKit Reporter: Dean Jackson <dino>
Component: New BugsAssignee: Dean Jackson <dino>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, eric.carlson, esprehn+autocc, ews, glenn, graouts, gyuyoung.kim, jdarpinian, jer.noble, kbr, kondapallykalyan, mmaxfield, noam, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 198948    
Attachments:
Description Flags
WIP
none
Patch
none
Patch
none
Patch none

Description Dean Jackson 2019-09-24 12:45:50 PDT
Compile ANGLE backend for iOS device
Comment 1 Dean Jackson 2019-09-24 12:46:52 PDT
Created attachment 379471 [details]
WIP
Comment 2 Dean Jackson 2019-10-31 15:52:43 PDT
Created attachment 382513 [details]
Patch
Comment 3 Dean Jackson 2019-11-07 15:05:51 PST
Created attachment 383077 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2019-11-07 15:06:24 PST
<rdar://problem/57000166>
Comment 5 Dean Jackson 2019-11-07 15:17:02 PST
Created attachment 383082 [details]
Patch
Comment 6 Build Bot 2019-11-07 15:18:15 PST
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 7 Myles C. Maxfield 2019-11-07 15:23:21 PST
Comment on attachment 383077 [details]
Patch

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

Cursory look seems okay. This is the beginning of a big project so it's difficult to be thorough...

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/ContextEAGL.h:44
> +class ContextEAGL : public ContextGL
> +{
> +  public:
> +    ContextEAGL(const gl::State &state,
> +               gl::ErrorSet *errorSet,
> +               const std::shared_ptr<RendererGL> &renderer);
> +};

Is this a stub for something that will come later? There's nothing here in this class.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:166
> +    return new IOSurfaceSurfaceEAGL(state, mContext, clientBuffer, attribs);

This seems so leaky :( :( :(

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:205
> +    config.nativeVisualID   = 0;
> +    config.nativeVisualType = 0;
> +    config.nativeRenderable = EGL_TRUE;

I'd appreciate a link for where these numbers came from

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/IOSurfaceSurfaceEAGL.mm:309
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wunused-variable"

This file is iOS-specific. Can't we just fix the problem instead of ignoring warnings?
Comment 8 Dean Jackson 2019-11-07 15:50:09 PST
Comment on attachment 383077 [details]
Patch

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

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/ContextEAGL.h:44
>> +};
> 
> Is this a stub for something that will come later? There's nothing here in this class.

Yeah. I once had a FIXME in here saying that ContextEAGL and ContextCGL could go away.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:166
>> +    return new IOSurfaceSurfaceEAGL(state, mContext, clientBuffer, attribs);
> 
> This seems so leaky :( :( :(

It does!

We release it from the calling side though.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/DisplayEAGL.mm:205
>> +    config.nativeRenderable = EGL_TRUE;
> 
> I'd appreciate a link for where these numbers came from

I don't think these matter for this implementation. If they are the default values, I should just remove these lines.

The values below are pretty obvious, I hope.

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/IOSurfaceSurfaceEAGL.mm:309
>> +#pragma clang diagnostic ignored "-Wunused-variable"
> 
> This file is iOS-specific. Can't we just fix the problem instead of ignoring warnings?

The problem is the iOS Simulator doesn't have texImageIOSurface, and ANGLE doesn't have something like the UNUSED_PARAM macro (as far as I can tell).
Comment 9 James Darpinian 2019-11-07 16:30:26 PST
Comment on attachment 383077 [details]
Patch

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

>>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/gl/eagl/IOSurfaceSurfaceEAGL.mm:309
>>> +#pragma clang diagnostic ignored "-Wunused-variable"
>> 
>> This file is iOS-specific. Can't we just fix the problem instead of ignoring warnings?
> 
> The problem is the iOS Simulator doesn't have texImageIOSurface, and ANGLE doesn't have something like the UNUSED_PARAM macro (as far as I can tell).

There's a macro ANGLE_UNUSED_VARIABLE in angle/src/common/debug.h
Comment 10 Dean Jackson 2019-11-08 11:01:33 PST
Committed r252245: <https://trac.webkit.org/changeset/252245>
Comment 11 Dean Jackson 2019-11-09 11:59:29 PST
Committed r252250: <https://trac.webkit.org/changeset/252250>