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

ImageSource and ImageSourceServices #759

Merged
merged 67 commits into from
Apr 29, 2021
Merged

ImageSource and ImageSourceServices #759

merged 67 commits into from
Apr 29, 2021

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Apr 16, 2021

Description of Change

This PR is trying to handle the case of loading some native "image" using the ImageSource. Right now, this PR only contains Android.

Initially, I tried returning the Drawable directly from the service, but this meant that there was no way to immediately release/dispose/free the bitmap. The GC would eventually get it - maybe. However, we can't rely on the GC as native bitmaps do not put enough pressure on the GC as well as sometimes we want to release them faster.

My solution here is to return a "result" object that contains both the actual drawable as well as some release logic. This logic is very abstract and is just an IDisposable. The actual logic for this dispose is not disposing the drawable directly because this is not up to us. Rather, it wraps the logic needed to reverse the request and comes from the service that created the result.

An example would be Glide that internally caches the bitmap and we have to indicate we are done with it:

  • Create: target = Glide.With(context)...Submit()
  • Dispose: Glide.With(context).Clear(target)

This logic is generic enough that it can work with any type of result since the service that caches the drawable is responsible for releasing it too.

class FileImageSource : ImageSource
{
    public string File { get; set; }
}

class Image : View
{
    public ImageSource Source { get; set; }
}

Android:

class GlideResult : IImageSourceServiceResult<Drawable>, IDrawable
{
    public GlideResult(Drawable drawable, Action dispose)
    {
        Value = drawable;
        _dispose = dispose;
    }

    // this is the actual drawable to display
    public Drawable Value { get; }

    // this is a way in which to release the image
    public void Dispose() => _dispose?.Invoke();
}

class FileImageSourceService
{
    public async Task<IImageSourceServiceResult<Drawable>?> GetDrawableAsync(IFileImageSource imageSource, Context context) 
    {
        var target = Glide
            .With(context)
            .Load(imageSource.File)
            .Submit();

        var drawable = target.GetAsync();

        return GlideResult(
            drawable: drawable,
            dispose: () => Glide.With(context).Clear(target));
    }
}

Usage

class ImageHandler : ViewHandler
{
    GlideResult _lastSource;

    public static void MapSource(ImageHandler handler, IImage image)
    {
        // clear out the old image
        handler.NativeView.SetImageDrawable(null);
        handler._lastSource?.Dispose();

        if (image.Source == null)
            return;

        // get the new one
        var result = await imageSourceService.GetDrawableAsync(image.Source);
        handler._lastSource = result;
        handler.NativeView.SetImageDrawable(result.Value);
    }
}

TODO

{
if (NativeView?.XamlRoot != null)
{
_lastScale = NativeView.XamlRoot.RasterizationScale;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a general place this value can get stored? Or maybe an extension method we can make that represents this flow of operations

var token = NAtiveView.WatchForRasterChanges(() => callback);
NativView.StopWatchingForRasterChanges(token)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. #887

This also happens on iOS. I assume Android does not change, but I am probably wrong. I also need to add that. Maybe.


namespace Microsoft.Maui.Hosting
{
class ImageSourceServiceProvider : MauiServiceProvider, IImageSourceServiceProvider
Copy link
Member

Choose a reason for hiding this comment

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


namespace Microsoft.Maui.Hosting.Internal
{
class FallbackLoggerFactory : ILoggerFactory
Copy link
Member

Choose a reason for hiding this comment

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

This is to indicate that the result is no longer valid and a new one should be obtained.
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

Cool!
Looks good

I think we should add back the preserve for now because the CG needs it for AppCenter tests and it's confusing when that one crashes for that reason

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Could we have also some image benckmark in the Benchmarks project?. I am thinking mostly in images from Url where things like Glide will have a huge impact but also, the metrics could help us to progress and improve (for example, with changes related with cache).

(Not necessary in this PR)


using (Stream stream = await imagesource.GetStreamAsync(cancelationToken).ConfigureAwait(false))
{
using (var decoder = new AndroidGIFImageParser(context, options.InDensity, options.InTargetDensity))
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, could fix several issues like xamarin/Xamarin.Forms#13629

Comment on lines +36 to +39
#if WINDOWS
var config = MauiWinUIApplication.Current.Services.GetService<IImageSourceServiceConfiguration>();
config?.SetImageDirectory(newValue?.ToString());
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably in the future will have more sense in something like ApplicationHandler if exists but looks good to me.

<PackageReference Include="Reloadify3000" Version="1.0.6" />
</ItemGroup>
<ItemGroup Condition="$(TargetFramework.StartsWith('MonoAndroid'))">
<PackageReference Include="Xamarin.Android.Glide" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what is the size of Glide library?

Copy link
Member Author

@mattleibow mattleibow Apr 29, 2021

Choose a reason for hiding this comment

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

1.8MB when the aar is embedded in the dll from the nugets. About 767KB of jar files. The linker/proguard should help a bit on this.

MapSourceAsync(handler, image).FireAndForget(handler);

public static async Task MapSourceAsync(ImageHandler handler, IImage image)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we include CancellationToken here to allow cancel the loading operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens inside:

var token = handler._sourceManager.BeginLoad();

The existing token is cancelled and starts a new source and returns the token to be used with UpdateSourceAsync

src/Core/src/Handlers/Image/ImageHandler.cs Outdated Show resolved Hide resolved
src/Core/src/Hosting/AppHost.cs Outdated Show resolved Hide resolved
[InlineData("red.png", "#FF0000")]
[InlineData("green.png", "#00FF00")]
[InlineData("black.png", "#000000")]
public async Task SourceInitializesCorrectly(string filename, string colorHex)
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@mattleibow mattleibow merged commit 4340ccf into main Apr 29, 2021
@mattleibow mattleibow deleted the dev/imagesource branch April 29, 2021 20:10
@ionixjunior ionixjunior mentioned this pull request May 5, 2021
12 tasks
PureWeen pushed a commit that referenced this pull request May 6, 2021
# Conflicts:
#	.nuspec/Microsoft.Maui.Controls.MultiTargeting.targets
#	src/Controls/src/Core/Controls.Core-net6.csproj
#	src/Controls/src/Core/HandlerImpl/Window.Impl.cs
#	src/Core/src/Core-net6.csproj
#	src/Core/src/TaskExtensions.cs
PureWeen pushed a commit that referenced this pull request May 17, 2021
# Conflicts:
#	.nuspec/Microsoft.Maui.Controls.MultiTargeting.targets
#	src/Controls/src/Core/Controls.Core-net6.csproj
#	src/Controls/src/Core/HandlerImpl/Window.Impl.cs
#	src/Core/src/Core-net6.csproj
#	src/Core/src/TaskExtensions.cs
PureWeen pushed a commit that referenced this pull request May 17, 2021
# Conflicts:
#	.nuspec/Microsoft.Maui.Controls.MultiTargeting.targets
#	src/Controls/src/Core/Controls.Core-net6.csproj
#	src/Controls/src/Core/HandlerImpl/Window.Impl.cs
#	src/Core/src/Core-net6.csproj
#	src/Core/src/TaskExtensions.cs
PureWeen pushed a commit that referenced this pull request May 18, 2021
# Conflicts:
#	.nuspec/Microsoft.Maui.Controls.MultiTargeting.targets
#	src/Controls/src/Core/Controls.Core-net6.csproj
#	src/Controls/src/Core/HandlerImpl/Window.Impl.cs
#	src/Core/src/Core-net6.csproj
#	src/Core/src/TaskExtensions.cs
@Pinox
Copy link

Pinox commented May 26, 2022

Hi @mattleibow ,

I am currently using FFImageloading with Xamarin Forms. I am checking out my dependencies in my current project for upgrade to MAUI. FFImageloading is a concern as I used it for fast image rendering on Android & iOS. As the Glide functionality is now integrated into MAUI for Android I have one less concern ;))

Looking at the FFImageloading repo it does not seem to be supported anymore by the authors. In nuget the latest download stats on the latest version is around 4.2 million downloads

https://www.nuget.org/packages/Xamarin.FFImageLoading/

So alot of developers are going to trip on this library. One of the other features I used FFImageloading for was Retry Count + Retry Delay for redundancy reasons.

image

This is a must on mobile devices that connects to unstable / intermittent internet connections to prevent for eg a listview from skipping images and displaying nothing.

Any chance that the MAUI team can support Retry Count & Retry Delay within MAUI ?

@pictos
Copy link
Contributor

pictos commented May 26, 2022

@Pinox can you open an issue proposing this feature?

@samhouts samhouts added area-image Image loading, sources, caching area-controls-image Image control labels Jul 20, 2023
@ghost ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Jul 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
@Eilon Eilon removed legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor area-image Image loading, sources, caching labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-image Image control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants