Skip to content

[Android] Fix: Prevent Duplicate MapSource Invocations on setting Image's Source to Local File Paths #28405

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

Merged
merged 2 commits into from
May 22, 2025

Conversation

bhavanesh2001
Copy link
Contributor

Description of Change

This PR addresses an side effect introduced by #24023 where setting the image source to a local file path triggers duplicate calls to the MapSource mapper. This behavior can lead to performance degradation—especially in image-heavy scenarios such as CollectionView or CarouselView.In such cases, multiple calls could potentially compound the load and result in a significant performance hit when images are loaded within ItemsViews.

Problem:
When the platform view attaches to the window, the handler checks if the image drawable is null and then calls UpdateValue(nameof(IImage.Source)) to reload the image. With local file paths, this can happen even if the image is already in the process of loading, causing MapSource to be fired multiple times unnecessarily.

Fix:
To mitigate this, introduced an IsLoading flag in the ImageSourceServiceResultManager. The flag is set to true when a load begins (via BeginLoad()) and reset to false upon load completion (via CompleteLoad()). In OnPlatformViewAttachedToWindow, we now check this flag before triggering a reload:

@bhavanesh2001 bhavanesh2001 requested a review from a team as a code owner March 14, 2025 10:19
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Mar 14, 2025
@jsuarezruiz jsuarezruiz added platform/android area-image Image loading, sources, caching labels Mar 14, 2025
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz added the perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label Mar 28, 2025
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 you include a test? Like tap a Button to load the image source, use the Image PropertyChanged event to check the source property and increase a counter. In that way, verify that MapSource is only invoked once.

 int sourceChangedCounter = 0;
 image.PropertyChanged += (sender, args) =>
 {
      if (args.PropertyName == nameof(Image.Source))
      {
          sourceChangedCounter++;
      }
 };

@bhavanesh2001
Copy link
Contributor Author

Thank you for the suggestion, @jsuarezruiz.

I could write a test similar to what you described, but I believe it wouldn’t accurately represent the issue I’m trying to address.

The line below invokes the MapSource method directly without raising a PropertyChanged event for the Source property:

imageHandler.UpdateValue(nameof(IImage.Source));

Since MapSource is triggered as part of the handler setup, and not through a change to Image.Source, subscribing to PropertyChanged and checking for Image.Source won’t capture that UpdateValue() call.

@bhavanesh2001
Copy link
Contributor Author

bhavanesh2001 commented Mar 28, 2025

To replicate the issue in the Sandbox app, follow the steps below:

1. Add Image to Resources

Add shopping_cart.png to the Resources/Raw folder.


2. Define UI in MainPage.xaml

<VerticalStackLayout Margin="20" x:Name="stack">
    <Button AutomationId="MauiImage" Text="To Local Storage" Clicked="Button_Clicked"/>
    <Button AutomationId="MauiImage1" Text="Load Images" Clicked="Button_Clicked1"/>
    
    <CollectionView x:Name="cauroselView">
        <CollectionView.ItemsLayout>
            <LinearItemsLayout Orientation="Horizontal" />
        </CollectionView.ItemsLayout>
        <CollectionView.ItemTemplate>
            <DataTemplate>
                <Image WidthRequest="300" Source="{Binding .}" />
            </DataTemplate>
        </CollectionView.ItemTemplate>
    </CollectionView>
</VerticalStackLayout>

3. Add Code-Behind in MainPage.xaml.cs

public partial class MainPage : ContentPage
{
    public MainPage()
    {
        InitializeComponent();
        Images = new List<string>();
    }

    public List<string> Images { get; set; }

    private async void Button_Clicked(object sender, EventArgs e)
    {
        for (int i = 0; i < 25; i++)
        {
            var resourceStream = await FileSystem.OpenAppPackageFileAsync("shopping_cart.png");
            var path = Path.Combine(FileSystem.AppDataDirectory, $"temp{i}.png");

            if (File.Exists(path))
                File.Delete(path);

            using (var fileStream = File.Create(path))
            {
                await resourceStream.CopyToAsync(fileStream);
                Images.Add(path);
            }
        }
    }

    private void Button_Clicked1(object sender, EventArgs e)
    {
        cauroselView.ItemsSource = Images;
    }
}

4. Set Breakpoint

Place a breakpoint at the following line in ImageHandler.Android.cs:

imageHandler.UpdateValue(nameof(IImage.Source));


5. Test the Behavior

  • Run the app.
  • Click "To Local Storage" to generate and save image files locally.
  • Click "Load Images" to bind the list of image paths to the CollectionView.
  • Scroll through the CollectionView.

Without the PR changes:

  • The UpdateValue() method will be invoked multiple times while scrolling.
  • This is unintended and incorrect behavior.

With the PR changes:

  • The UpdateValue() method will not be invoked during scrolling.

@sairam-the-dev
Copy link

Images inside a CollectionView or CarouselView are causing lot of issues for us. Laggy scroll, poor performance... This seems like it will help improve on that. @rmarinho @PureWeen Could you take a look at this?

@bcaceiro
Copy link

bcaceiro commented May 5, 2025

This can also explain my degradating performance in my CV's with local images

@PureWeen PureWeen added this to the .NET 9 SR8 milestone May 5, 2025
@PureWeen PureWeen moved this from Todo to Ready To Review in MAUI SDK Ongoing May 5, 2025
@PureWeen PureWeen added the p/0 Work that we can't release without label May 9, 2025
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Maybe use another field for the value and also is this testable?

@github-project-automation github-project-automation bot moved this from Ready To Review to Changes Requested in MAUI SDK Ongoing May 16, 2025
@bhavanesh2001 bhavanesh2001 force-pushed the improve_android_image branch from 685db9f to 95732d4 Compare May 17, 2025 22:29
@PureWeen PureWeen requested a review from mattleibow May 20, 2025 20:25
@PureWeen PureWeen moved this from Changes Requested to Ready To Review in MAUI SDK Ongoing May 20, 2025
@PureWeen PureWeen changed the base branch from main to inflight/current May 22, 2025 18:15
@PureWeen PureWeen merged commit 70224bc into dotnet:inflight/current May 22, 2025
1 of 2 checks passed
@github-project-automation github-project-automation bot moved this from Ready To Review to Done in MAUI SDK Ongoing May 22, 2025
github-actions bot pushed a commit that referenced this pull request May 22, 2025
…ge's Source to Local File Paths (#28405)

* add extra condition

* use _sourceCancellation
github-actions bot pushed a commit that referenced this pull request May 30, 2025
…ge's Source to Local File Paths (#28405)

* add extra condition

* use _sourceCancellation
github-actions bot pushed a commit that referenced this pull request May 30, 2025
…ge's Source to Local File Paths (#28405)

* add extra condition

* use _sourceCancellation
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-image Image loading, sources, caching community ✨ Community Contribution p/0 Work that we can't release without perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) platform/android
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants