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.

Sign up for GitHub

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

Fix iOS image loading process outputting alpha-premultiplied colours #6454

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+29 −12
Draft

Fix iOS image loading process outputting alpha-premultiplied colours #6454

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions osu.Framework.iOS/Graphics/Textures/IOSTextureLoaderStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Buffers;
using System.IO;
using CoreGraphics;
using CoreImage;
using Foundation;
using osu.Framework.Graphics.Textures;
using osu.Framework.IO.Stores;
using SixLabors.ImageSharp;
using UIKit;
using SixLabors.ImageSharp.PixelFormats;

namespace osu.Framework.iOS.Graphics.Textures
{
Expand All @@ -19,28 +21,43 @@ public IOSTextureLoaderStore(IResourceStore<byte[]> store)
{
}

protected override Image<TPixel> ImageFromStream<TPixel>(Stream stream)
protected override unsafe Image<TPixel> ImageFromStream<TPixel>(Stream stream)
{
using (var nativeData = NSData.FromStream(stream))
{
if (nativeData == null)
throw new ArgumentException($"{nameof(Image)} could not be created from {nameof(stream)}.");

using (var uiImage = UIImage.LoadFromData(nativeData))
using (var ciImage = CIImage.FromData(nativeData))
{
if (uiImage == null) throw new ArgumentException($"{nameof(Image)} could not be created from {nameof(stream)}.");
if (ciImage == null) throw new ArgumentException($"{nameof(Image)} could not be created from {nameof(stream)}.");

int width = (int)uiImage.Size.Width;
int height = (int)uiImage.Size.Height;
int width = (int)ciImage.Extent.Width;
int height = (int)ciImage.Extent.Height;

// TODO: Use pool/memory when builds success with Xamarin.
// Probably at .NET Core 3.1 time frame.
byte[] data = new byte[width * height * 4];
using (CGBitmapContext textureContext = new CGBitmapContext(data, width, height, 8, width * 4, CGColorSpace.CreateDeviceRGB(), CGImageAlphaInfo.PremultipliedLast))
textureContext.DrawImage(new CGRect(0, 0, width, height), uiImage.CGImage);
RgbaVector[] dataInVectors = ArrayPool<RgbaVector>.Shared.Rent(width * height);
byte[] dataInBytes = ArrayPool<byte>.Shared.Rent(width * height * 4);

var image = Image.LoadPixelData<TPixel>(data, width, height);
fixed (RgbaVector* ptr = dataInVectors)
{
using (var context = CIContext.Create())
context.RenderToBitmap(ciImage, (IntPtr)ptr, width * 16, ciImage.Extent, CIImage.FormatRGBAf, CGColorSpace.CreateDeviceRGB());
}

// unapply alpha pre-multiplication resulted by CGBitmapContext.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a quick read it doesn't look like there's a CGBitmapContext anymore?

Also just a general question: did you use some other code as reference for this? It would be good to have some links to other similar implementation to reassure me that this isn't completely back to front (because I feel like it might be, and that the correct method is to adjust things so we can set the src/dest blend to support this at a metal level).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a quick read it doesn't look like there's a CGBitmapContext anymore?

Yeah this comment is stale, should mention CIContext instead.

Also just a general question: did you use some other code as reference for this? It would be good to have some links to other similar implementation to reassure me that this isn't completely back to front (because I feel like it might be, and that the correct method is to adjust things so we can set the src/dest blend to support this at a metal level).

This is not referenced from anywhere else, it was written as such after various attempts of internet searching to achieve the desired behaviour, specifically after being aware of CIContext in a random stack overflow thread (I should also mention that CGBitmapContext supports floating-point components but the numbers are completely imprecise, seemingly converted from premultiplied byte values).

As to whether this is back to front or not, it is back to front in terms of iOS/Metal most likely. As I've read more about this before submitting this PR it does seem like the correct approach is to configure Metal to blend premultiplied colours more correctly, but I've gone with this PR first as I have a feeling it will not be a simple one at all. The masking/texture shaders will probably have to be thoroughly re-checked to handle premultiplied colours correctly, alongside figuring out what it takes to make Metal blend premultiplied colours.

I'll investigate that today and see where it leads me.

for (int i = 0; i < dataInVectors.Length; i++)
{
RgbaVector v = dataInVectors[i];
dataInBytes[i * 4 + 0] = (byte)Math.Round(v.A == 0 ? 0 : v.R / v.A * 255);
dataInBytes[i * 4 + 1] = (byte)Math.Round(v.A == 0 ? 0 : v.G / v.A * 255);
dataInBytes[i * 4 + 2] = (byte)Math.Round(v.A == 0 ? 0 : v.B / v.A * 255);
dataInBytes[i * 4 + 3] = (byte)Math.Round(v.A * 255);
}

var image = Image.LoadPixelData<TPixel>(dataInBytes, width, height);

ArrayPool<byte>.Shared.Return(dataInBytes);
ArrayPool<RgbaVector>.Shared.Return(dataInVectors);
return image;
}
}
Expand Down
Loading