Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[metalperformanceshaders] Fix MPSImageLanczosScale base class change #3170

Merged

Conversation

spouliot
Copy link
Contributor

@spouliot spouliot commented Jan 6, 2018

Sadly this creates a breaking change since the ScaleTransform
property was re-introduced with an incorrect signature in the new
base class MPSImageScale

Unless someone has an idea how to avoid it (I don't see an option)
then we'll have to document it in the 15.7 release notes.

Sadly this creates a breaking change since the `ScaleTransform`
property was re-introduced with an incorrect signature in the new
base class `MPSImageScale`

Unless someone has an idea how to avoid it (I don't see an option)
then we'll have to document it in the 15.7 release notes.
@monojenkins
Copy link
Collaborator

Build success

@rolfbjarne
Copy link
Member

I'm fairly certain you can have the method in both classes with different signatures.

It's quite ugly, but it would not be a breaking binary change:

class MPSImageLanczosScale : MPSImageScale {
    public new virtual MPSScaleTransform? ScaleTransform { get; set; } // existing correct signature
}

class MPSImageScale {
    public virtual MPSScaleTransform ScaleTransform { get; set; } // old broken signature
}

Another issue is that the property with the correct signature has an incorrect implementation, the non-null setter path never calls the native _SetScaleTransform method to set the value, it writes the struct data to native memory, and then just immediately frees the native memory:

if (value.HasValue) {
IntPtr ptr = Marshal.AllocHGlobal (size_of_scale_transform);
try {
Marshal.StructureToPtr<MPSScaleTransform> (value.Value, ptr, false);
}
finally {
Marshal.FreeHGlobal (ptr);
}
} else {
_SetScaleTransform (IntPtr.Zero);
}

@spouliot spouliot added the do-not-merge Do not merge this pull request label Jan 9, 2018
@spouliot
Copy link
Contributor Author

spouliot commented Jan 9, 2018

@rolfbjarne good catch! it's not likely an API used commonly (which is good if we need to do a breaking change).

As for signature we want MPSImageScale to have the correct one too.

@spouliot
Copy link
Contributor Author

spouliot commented Jan 9, 2018

There are also other subclasses of MPSImageScale that makes the above hack impossible to use :|

Adding it to it's base class, MPSUnaryImageKernel, seems more likely to cause further harm (at least confusion).

@spouliot spouliot removed the do-not-merge Do not merge this pull request label Jan 9, 2018
@monojenkins
Copy link
Collaborator

Build success

@spouliot spouliot merged commit f9ceb5c into xamarin:master Jan 9, 2018
@spouliot spouliot deleted the MPSImageLanczosScale-base-type-change branch January 9, 2018 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants