<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add support to attribute ownership of ANGLE Metal's resources"
   href="https://bugs.webkit.org/show_bug.cgi?id=233941#c10">Comment # 10</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add support to attribute ownership of ANGLE Metal's resources"
   href="https://bugs.webkit.org/show_bug.cgi?id=233941">bug 233941</a>
              from <span class="vcard"><a class="email" href="mailto:kkinnunen@apple.com" title="Kimmo Kinnunen <kkinnunen@apple.com>"> <span class="fn">Kimmo Kinnunen</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=446442&action=diff" name="attach_446442" title="Patch">attachment 446442</a> <a href="attachment.cgi?id=446442&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=446442&action=review">https://bugs.webkit.org/attachment.cgi?id=446442&action=review</a>

<span class="quote">> Source/ThirdParty/ANGLE/include/EGL/eglext_angle.h:181
> +#define EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE 0x33B0</span >

Let's put 0x34A7 for now (reserved for ANGLE)

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/Caps.h:647
> +    bool createContextOwnershipIdentity = false;</span >

createContextOwnershipIdentity*Metal* ?

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:235
> +EGLint GetOwnershipIdentity(const egl::AttributeMap &attribs)</span >

This file is for *all* implementations.
Ownership identity is a property of the Metal backend.
You need to move this to ContextMtl

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/Context.cpp:237
> +    return static_cast<EGLint>(attribs.getAsInt(EGL_CONTEXT_OWNERSHIP_IDENTITY_METAL_ANGLE, EGL_FALSE));</span >

getAsInt already returns int.
The default value should be 0, not EGL_FALSE

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/State.h:105
> +          EGLint owwnershipIdentity);</span >

Typo, but anyway needs to move away from the State.

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:198
> +      mContextDevice(</span >

here you would add  as the attribs: ContextMtl(.., AttributeMap& attribs) 
mContextDevice(GetOwenershipIdentity(attribs))

and in GetOwenershipIdentity you'd #ifdef

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:2449
> +#endif</span >

Maybe remove, read the token off from attributes in the constructor and pass it to ContextDevice

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.h:28
> +    ContextDevice(task_id_token_t ownershipIdentity) { mOwnershipIdentity = ownershipIdentity; }</span >

Maybe:
1) remove ifdef
2) set the parameter as GLint
3)  move the implementation to .mm file.

In implementation, do following

ContextDevice(GLint ownerIdentity)
{
#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
    auto ownerIdentity = static_cast<task_id_token_t>(ownerIdentity);
    if (ownerIdentity != TASK_ID_TOKEN_NULL)
    {
         kr = mach_port_mod_refs(mach_task_self(), ownerIdentity, MACH_PORT_RIGHT_SEND, 1);
        .... check kr, etc. 
       mOwnerIdentity = ownerIdentity;
    }
#endif
}
in ~ContextDevice, unref the task token.

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:18
> +#pragma mark - Context Device implementation</span >

I don't think you need a pragma mark categorisation for a 50 line implementation?

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:36
> +#if USE_OWNERSHIP_IDENTITY_PRIVATE</span >

so remove the extra ifdefs
the code should be 

auto texture = [get() newTextureWithDescriptor:descriptor];
setOwnerWithIdentity(texture);

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_context_device.mm:124
> +}</span >

Here you would #if:

void ContextDevice::setOwnerWithIdentity(id<MTLResource> resource)
{
#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY
    setOwnerWithIdentity(resource, mOwnerIdentity);
#endif
}

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_format_table_autogen.mm:304
> +    id<MTLDevice> device = display->getMetalDevice();</span >

these probably are now somehow wrong.
Anyway, we shouldn't modify the autopen tables unless the generator is modified.
I don't see the python files modified.

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_ownership_identity.h:21
> +#import "libANGLE/renderer/metal/mtl_ownership_identity_private.h"</span >

Is it useful to have the thing implemented as:
 - if MTLResource_Private.h is missing, don't use
 - if mtl_ownership_identity_private.h is missing, don't use

Can we just remove this file and use the define: ANGLE_USE_METAL_OWNERSHIP_IDENTITY.
Then we make sure by default, ANGLE_USE_METAL_OWNERSHIP_IDENTITY is only defined if
 1) os version is correct
 2) internal headers are visible
 3) user has requested the feature
 (e.g. this define is what common/apple/apple_platform.h would set, see below)

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_ownership_identity_private.h:36
> +</span >

This file could be called mtl_resource_spi.h

Contents would be:

#if ANGLE_USE_METAL_OWNERSHIP_IDENTITY

#import <Metal/Metal.h>
#import <Metal/MTLResource_Private.h>
#import <mach/mach_types.h>

namespace rx
{

....

}

#endif

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_task_identity_token.h:21
> +#define ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY 0</span >

I think this could be moved to common/apple/apple_platform.h
I think additionally you could have the __has_include(<Metal/MTLResource_Private.h>) here

There you could have additionally

  #define ANGLE_USE_METAL_OWNERSHIP_IDENTITY (ANGLE_HAVE_MTLRESOURCE_SET_OWNERSHIP_IDENTITY && defined(ANGLE_ENABLE_METAL_OWNERSHIP_IDENTITY))

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:224
> +                if ([[metalDevice name] rangeOfString:it.trademark].location != NSNotFound)</span >

might be unneeded change?

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:238
> +    return angle::GetVendorIDFromMetalDeviceRegistryID([device registryID]);</span >

might be unneeded change?

<span class="quote">> Source/ThirdParty/ANGLE/src/libANGLE/validationEGL.cpp:1634
>  </span >

Here you would validate that the passed identity is a task_id_token_t (e.g. at least that it's a send right? I'm not 100% sure) or null token.
I don't know yet how to do that, but we want to be relatively sure that there's no error when we ref it</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>