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

Fix animated gifs in Release builds #22874

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Jun 5, 2024

Description of Change

I had to change the code a bit to prevent the linker from removing interfaces from types or something that I do not yet understand.

The old code would pass a URI to Glide and Glide would return a Drawable that may or may not be IAnimatable - for example, it could be AnimatedImageDrawable.

In Debug builds, the condition drawable is IAnimatable would return true, but in Release builds, this will be false. Checking the type, it was a Java AnimatedImageDrawable. The dotnet type is a DrawableInvoker since the dotnet type was linked out. This is a good thing because we can never use nor need the dotnet type. All we want is the Java interface.

@simonrozsival @jonathanpeppers does this make any sense to you as to why this would happen like this?

Issues Fixed

Fixes #12974

@mattleibow mattleibow requested a review from a team as a code owner June 5, 2024 23:14
@mattleibow mattleibow requested review from rmarinho and PureWeen June 5, 2024 23:14
@@ -29,7 +30,20 @@ public static void UpdateAspect(this ImageView imageView, IImage image)

public static void UpdateIsAnimationPlaying(this Drawable? drawable, IImageSourcePart image)
Copy link
Member

Choose a reason for hiding this comment

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

I had to fix something similar earlier this year: dotnet/android#8594 I think you can fix this with a few [DynamicDependency] attributes to this method:

[DynamicDependency(nameof(IAnimatable))]
[DynamicDependency(nameof(AnimationD)rawable)]
[DynamicDependency(nameof(GifDrawable))]
[DynamicDependency(nameof(AnimatedImageDrawable))]

Copy link
Member

Choose a reason for hiding this comment

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

Does no C# code use any of the above types? What @simonrozsival suggests seems like the right idea.

Something similar also happens here:

Since no C# code uses NinePatchDrawable it's linked away. Mono.Android.dll and all the AndroidX libraries mark themselves trimmable.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that I don't know what types are being used - I know of some but it could be any type of drawable. I would have to list all of the known ones and then it would still not be super perfect because of third parties.

Also, since we don't use the type in C# - all the loading code is in Java now - we are otentially forcing a whole bunch of C# types to exist when we don't really need it.

Copy link
Member

@simonrozsival simonrozsival Jun 10, 2024

Choose a reason for hiding this comment

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

It doesn't seem like [DynamicDependency] is applicable in this case then. Doing the JavaCast might be the best way after all. Hopefully Proguard won't be messing up the Java side of things :)

@jsuarezruiz jsuarezruiz added the area-image Image loading, sources, caching label Jun 6, 2024
Comment on lines +44 to +47
catch
{
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of an empty try-catch with no logging.

Does a [DynamicDependency] like @simonrozsival suggested also solve it? That would have no performance cost, and I think things would still be trimmed away if you had 0 animated gifs in your app.

Copy link
Member Author

Choose a reason for hiding this comment

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

I logged an issue with a feature request or something: dotnet/android#9038

commit db8a3bd
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 10 13:46:16 2024 +0800

    Add android

commit 0bd2d65
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 10 12:16:34 2024 +0800

    oops!

commit 647e5f5
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 18:15:40 2024 +0800

    Almost

commit c3ea650
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 16:17:58 2024 +0800

    sadfasdf

commit d9a398f
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 13:56:34 2024 +0800

    screenshots

commit 4b3c01f
Merge: 595ff03 e94b364
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 13:54:52 2024 +0800

    Merge branch 'main' into dev/gif-in-release

commit 595ff03
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 21:00:52 2024 +0800

    Update ImageUITests.cs

commit 2c20fe0
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 19:05:23 2024 +0800

    Make some tests

commit 1e82e8b
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 18:44:18 2024 +0800

    Take 2?

commit d576c82
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 07:11:17 2024 +0800

    Fix animated gifs in Release builds
@mattleibow mattleibow force-pushed the dev/gif-in-release branch from db8a3bd to e67c4c4 Compare June 18, 2024 09:30
@mattleibow mattleibow changed the base branch from main to release/8.0.1xx-sr6 June 18, 2024 09:30
@Redth
Copy link
Member

Redth commented Jun 18, 2024

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@mattleibow mattleibow merged commit a8367fc into release/8.0.1xx-sr6 Jun 18, 2024
4 of 6 checks passed
@mattleibow mattleibow deleted the dev/gif-in-release branch June 18, 2024 18:43
PureWeen added a commit that referenced this pull request Jun 20, 2024
PureWeen added a commit that referenced this pull request Jun 20, 2024
Redth added a commit that referenced this pull request Jun 25, 2024
* Size and SizeF should not throw on NaN

* Fix Release Versioning

* Upgrade from 1.5.1 to 1.5.4

* SwipeView Fix #22580 (#22741)

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>

* Update vscode extension recommendations

* [XC] Fix SimplifyTypeExtensionVisitor (#23043)

* Add test

* Fix the visitor

* Add SR6 to issue template (#23071)

* Make sure the main branch is using .NET 8 SDK (#23077)

* Make sure the main branch is using .NET 8 SDK

The main branch needs to use the .NET 8 SDK because we are building the .NET 7 TFMs as well. The .NET 9 SDK does not support .NET 7 TFMs anymore.

* no previews!

* Setup preview versioning for SR6.1

* Remove compat appium tests

Since we're moving in a different direction for moving these over in #22635 these projects (and the Issue11853 test, since it's in the other PR) can be deleted.

* [main] Update arcade and xharness (#22981)

* Update arcade

* Update dotnet-tools.json

* Update xharness
# Conflicts:
#	.config/dotnet-tools.json
#	eng/Version.Details.xml
#	eng/Versions.props

* Remove more references to removed projects

* Make titlebar button foreground colors use app theme

* Update dependencies from https://github.com/dotnet/xharness build 20240612.3 (#23088)

Microsoft.DotNet.XHarness.CLI , Microsoft.DotNet.XHarness.TestRunners.Common , Microsoft.DotNet.XHarness.TestRunners.Xunit
 From Version 9.0.0-prerelease.24311.2 -> To Version 9.0.0-prerelease.24312.3

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Add additional logging for PopLifeCycle

* Remove more legacy pipeline bits

* [iOS] Fixed NRE after calling ViewCell.ForceUpdateSize (#23094)

* [iOS] Fixed NRE after calling ViewCell.ForceUpdateSize

* - add test

---------

Co-authored-by: Shane Neuville <shneuvil@microsoft.com>

* Add x:DataType to the carousel view UI tests (#23113)

* Bump to Android 34.0.113

Context: https://github.com/dotnet/android/releases/34.0.113
Changes: dotnet/android@34.0.79...34.0.113

I noticed the Android workload version used in dotnet/maui/main was a couple service releases old.

We should use the latest one, as it has a memory leak fix for `Post()`.

* Squashed commit of the following: (#22874)

commit db8a3bd
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 10 13:46:16 2024 +0800

    Add android

commit 0bd2d65
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 10 12:16:34 2024 +0800

    oops!

commit 647e5f5
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 18:15:40 2024 +0800

    Almost

commit c3ea650
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 16:17:58 2024 +0800

    sadfasdf

commit d9a398f
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 13:56:34 2024 +0800

    screenshots

commit 4b3c01f
Merge: 595ff03 e94b364
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 13:54:52 2024 +0800

    Merge branch 'main' into dev/gif-in-release

commit 595ff03
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 21:00:52 2024 +0800

    Update ImageUITests.cs

commit 2c20fe0
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 19:05:23 2024 +0800

    Make some tests

commit 1e82e8b
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 18:44:18 2024 +0800

    Take 2?

commit d576c82
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 07:11:17 2024 +0800

    Fix animated gifs in Release builds

* Fix loaded so it fires on second subscription (#23095)

* Fix loaded so it fires on second subscription

* Update VisualElement.cs

* Make sure the view is still alive after posting (#23114)

I saw in some of my local testing the shape view may
actually be disposed if you write code or when you
run the device tests.

When we come back after the UI loop, make sure the
platform view is still alive.

This should not happen if the view is attached to the
UI since it is still being used, but if you happen to
navigate or close some popup it may occur.

* [iOS] TapGestureRecognizer should not fire when view is not enabled (#23049)

* Don't receive touch when view is disabled

* Add UITest for Single Tap enabled/disabled

This removes the first pass unit test version which isn't really testing the scenario properly.

* Fix element automation id

* Don't fire tap for windows if control disabled

* Make failure case text less confusing

* [Tests] Update to Appium 5.0.0 (#23118)

* Update to appium 5.0.0

Their driver has some removed methods (LaunchApp/CloseApp) which seem to be now implemented for windows.

Also removed an extra explicit ref to system.drawing.common since it comes in transitively from appium.webdriver package (and the version was causing issues since the newer appium.webdriver uses a newer version).

* Fix windows launchapp/closeapp for newer appium

In Appium's dotnet driver in v5.0.0 they removed `LaunchApp` from the driver: appium/dotnet-client#766

The deprecation suggests using `ActivateApp` as an alternative, but that throws an error saying it's not implemented on the windows driver.

Curiously, `CloseApp` which was also marked as deprecated was _moved_ from the base appium driver to the `WindowsDriver` here: appium/dotnet-client#773

I think the same treatment should have been done to the `LaunchApp` method since there's no alternative in windows.

For now the workaround is to invoke the command manually:
`windowsDriver.ExecuteScript("windows: launchApp", [_app.GetAppId()]);`

* Use correct interface type in FrameRenderer (#23124)

* Use correct interface type in FrameRenderer (#23124) (#23146)

* Squashed commit of the following: (#19629)

commit f6c3bfa
Merge: d0b3f84 3143629
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 17 19:58:40 2024 +0800

    Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet

commit d0b3f84
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Tue Jun 11 05:00:38 2024 +0800

    Fix the older macOS testing

commit 3dc6b4d
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Mon Jun 10 23:40:40 2024 +0800

    macOS 13 things

commit e3e48e4
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 02:15:55 2024 +0800

    docs

commit 693d79c
Merge: 3dbce49 a3c872d
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 02:08:31 2024 +0800

    Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet

commit 3dbce49
Merge: 17ea9c6 93a1bc4
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 02:06:16 2024 +0800

    Merge remote-tracking branch 'origin/main' into dev/macos-actionsheet

commit 17ea9c6
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Jun 8 02:05:42 2024 +0800

    Fix the tests

commit 026da41
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Fri Jun 7 03:56:26 2024 +0800

    fixes

commit d2b85d5
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 23:58:33 2024 +0800

    namespaces

commit 9fa77cd
Merge: 3f9596b 9d71d32
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Thu Jun 6 23:57:00 2024 +0800

    Merge branch 'main' into dev/macos-actionsheet

    # Conflicts:
    #	src/Controls/samples/Controls.Sample.UITests/Test.cs
    #	src/Controls/src/Core/Platform/AlertManager/AlertManager.iOS.cs
    #	src/Controls/tests/TestCases.Shared.Tests/Tests/Concepts/AlertsGalleryTests.cs
    #	src/Controls/tests/TestCases/Concepts/AlertsGalleryPage.cs
    #	src/TestUtils/src/UITest.Appium/AppiumCatalystApp.cs
    #	src/TestUtils/src/UITest.Appium/HelperExtensions.cs

commit 3f9596b
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Sat Dec 30 09:29:31 2023 +0200

    Add some UI tests

    Done: iOS/macOS/Android. TODO: Windows

commit 68c930f
Author: Matthew Leibowitz <mattleibow@live.com>
Date:   Tue Dec 19 18:15:51 2023 +0200

    macOS does not use PopoverPresentationController

    Fixes #18156

* [ios/catalyst] fix more cycles in `NavigationPage`

Fixes: #21453
Context: #22810

In #22810, a leak in `NavigationPage` was fixed for the case:

    Application.Current.MainPage = new NavigationPage(new Page1());
    Application.Current.MainPage = new Page2();

However, it does *not* work for the case:

    await Navigation.PushModalAsync(new NavigationPage(new Page1()));
    await Navigation.PopModalAsync();

I could reproduce this problem in `MemoryTests.cs`.

There were still a few cycles in `NavigationRenderer`:

* `NavigationRenderer` -> `VisualElement _element` -> `NavigationRenderer`

* `NavigationRenderer` -> `Page Current` -> `NavigationRenderer`

* `NavigationRenderer` -> `ViewHandlerDelegator<NavigationPage> _viewHandlerWrapper` ->  `TElement _element` -> `NavigationRenderer`

After fixing these cycles, the test passes.

`ViewHandlerDelegator` was slightly tricky, as I had to make a
`_tempElement` variable and *unset* it immediately after use.

* Only change ViewHandlerDelegator for iOS/Catalyst

* [ios] fix leak in ListView *Cells (#23143)

* [ios] fix leak in ListView *Cells

Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2064274

After fixing #22867 for `CollectionView`, there was still a problem with
`ListView`. The following types were leaking in the sample app:

* `UIKit.UITableView`
* `Microsoft.Maui.Controls.Handlers.Compatibility.ViewCellRenderer.ViewTableCell`
* `Microsoft.Maui.Platform.MauiLabel`
* `Microsoft.Maui.Platform.MauiImageView`
* `Microsoft.Maui.Platform.WrapperView`

After a lot of debugging, we found that `Cell` was holding a reference
to the `UITableView`. This pointed *up* in the hierarchy, creating
a cycle.

I updated an existing device test to ensure the problem is solved.

* Fix missing handlers in tests

* Remove assertion

* Fix UITableView reference

* Update a different test

* Can't enumerate while adding

* Missed a null check

* [Housekeeping] Added UI Test to validate project template (#18567)

* Added template UI Test

* Added pending snapshot

* Updated snapshot

* Generate snapshots for all the platforms

* Added pending snapshots

* Updated snapshot

* More changes

* Updated droid snapshot

* More changes

* Updated snapshots

* Added sample path

* Update Issue19509.cs

* Update Issue19509.cs

---------

Co-authored-by: Shane Neuville <shneuvil@microsoft.com>

* Optimize resetting gesture recognizers (#19987)

* reduce LINQ usage

* Remove useless check that will always be false

* use HashSet

* reduce enumerations and casting

* reduce LINQ usage

* only cast once

* add benchmarks

* fix build

---------

Co-authored-by: Edward Miller <symbiogenisis@outlook.com>

* [Windows] Fix ListView insert not working properly (#22746)

* Add test

* Remove force layout update during collection changed event

* Adjust await

* Update ListViewTests.Windows.cs

---------

Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>

* Fix <ApplicationTitle> encoding in maui templates (#22084)

* Makes <ApplicationTitle> use a custom symbol instead of the default name parameter to ensure the name is XML encoded where necessaru
* Adds new tests for various encodings and special characters
---------

Co-authored-by: Eilon Lipton <Eilon@users.noreply.github.com>

* Fixed a null check #15102 (#23170)

* [ios/catalyst] fix memory leak in TabbedPage (#23166)

* [ios/catalyst] fix memory leak in TabbedPage

Context: #23164

Just the same way as `NavigationPage` in #23164, `TabbedPage` also has
a memory leak caused by the cycle:

* `TabbedPage` -> `TabbedRenderer` -> `VisualElement _element;` -> `TabbedPage

I could add a new `[Theory]` in `MemoryTests.cs` to see the issue.

This PR fixes the memory leak by breaking the cycle in `TabbedRenderer`.

* Ignore test on Windows

I also cleaned up the Task.Delay()

* [iOS] Set PlatformGraphicsView to transparent input (#23208)

* Avoid JavaCast and exceptions and ask Java (#23215)

* Wire RefreshView up to our xplat layout workflow (#23169)

* Use better layout/measure path with refreshview

* - fix naming

* - set RefreshView content to maui compatible container

* - add test

* - fix null operator

* Update Issue23029.xaml.cs

* - fix content panel so it removes previous content

* - add additional check

* Fix Merge

---------

Co-authored-by: Matthew Leibowitz <mattleibow@live.com>
Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: redth <jondick@gmail.com>
Co-authored-by: Šimon Rozsíval <simon@rozsival.com>
Co-authored-by: Rui Marinho <me@ruimarinho.net>
Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Vitaly Knyazev <VitalyKnyazev@users.noreply.github.com>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: Jonathan Dick <jodick@microsoft.com>
Co-authored-by: Edward Miller <symbiogenesis@outlook.com>
Co-authored-by: Edward Miller <symbiogenisis@outlook.com>
Co-authored-by: Mike Corsaro <corsarom@gmail.com>
Co-authored-by: Michael Yanni <MiYanni@microsoft.com>
Co-authored-by: Eilon Lipton <Eilon@users.noreply.github.com>
Co-authored-by: Tim Miller <drasticactions@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net MAUI 7 Android, Image Gif Animation Not Playing in Release Mode, Normal in Debug Mode
6 participants