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 input starvation issues on resource-constrained devices. #15137

Merged
merged 3 commits into from
Apr 20, 2024

Conversation

grokys
Copy link
Member

@grokys grokys commented Mar 26, 2024

What does the pull request do?

On resource-constrained devices, input starvation can occur more commonly than on a typical desktop device, and when it happens it's important that the device retains some semblance of interactivity. This PR makes two small tweaks which improve this situation:

  • Reduces the input starvation timeout from 1 second to 0.2 seconds, so that input is processed at least 5 times per second
  • Ensures that the input starvation code is run from ICompositorScheduler.CommitRequested by passing false for the now parameter to ScheduleRender

grokys added 2 commits March 26, 2024 14:57
1 second was too long when running on resource-constrained devices.
Using `ScheduleRender(true)` here prevented the input starvation code from running.
@grokys grokys requested a review from kekekeks March 26, 2024 14:03
@@ -16,7 +16,7 @@ internal partial class MediaContext : ICompositorScheduler
private TimeSpan _inputMarkerAddedAt;
private bool _isRendering;
private bool _animationsAreWaitingForComposition;
private const double MaxSecondsWithoutInput = 1;
private const double MaxSecondsWithoutInput = 0.2;
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make it configurable, otherwise we'll risk running into issues with XPF too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any preference as to where this configuration should be placed?

Copy link
Member

Choose a reason for hiding this comment

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

We probably need a separate options class. DispatcherOptions, maybe?

We should also consider runtimeconfig.json-based settings at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Please check that the naming and doc comments are to your liking.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046640-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

And allow setting the input starvation timeout there.
@@ -0,0 +1,21 @@
using System;

namespace Avalonia.Threading;
Copy link
Member

Choose a reason for hiding this comment

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

We usually keep other **Options classes in Avalonia namespace.
SkiaOptions as an example: https://github.com/AvaloniaUI/Avalonia/blob/master/src/Skia/Avalonia.Skia/SkiaOptions.cs#L4

So it won't require any extra usings for app builder.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't really seem to be consistent in this. A couple of exceptions I've seen are:

  • Avalonia.Rendering.Composition.CompositionOptions
  • Avalonia.LinuxFramebuffer.LinuxFramebufferPlatformOptions

However, yes I can move this class to the root namespace.

/// <summary>
/// AppBuilder options for configuring the <see cref="Dispatcher"/>.
/// </summary>
public class DispatcherOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class DispatcherOptions
public record DispatcherOptions

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we need a record here? AFAICS we don't need any record features and using a record will bloat the size of the code:

[NullableContext(1)]
    [Nullable(0)]
    public class DispatcherOptions : IEquatable<DispatcherOptions>
    {
        [CompilerGenerated]
        private TimeSpan <InputStarvationTimeout>k__BackingField;

        [CompilerGenerated]
        protected virtual Type EqualityContract
        {
            [CompilerGenerated]
            get
            {
                return typeof(DispatcherOptions);
            }
        }

        public TimeSpan InputStarvationTimeout
        {
            [CompilerGenerated]
            get
            {
                return <InputStarvationTimeout>k__BackingField;
            }
            [CompilerGenerated]
            set
            {
                <InputStarvationTimeout>k__BackingField = value;
            }
        }

        [CompilerGenerated]
        public override string ToString()
        {
            StringBuilder stringBuilder = new StringBuilder();
            stringBuilder.Append("DispatcherOptions");
            stringBuilder.Append(" { ");
            if (PrintMembers(stringBuilder))
            {
                stringBuilder.Append(' ');
            }
            stringBuilder.Append('}');
            return stringBuilder.ToString();
        }

        [CompilerGenerated]
        protected virtual bool PrintMembers(StringBuilder builder)
        {
            RuntimeHelpers.EnsureSufficientExecutionStack();
            builder.Append("InputStarvationTimeout = ");
            builder.Append(InputStarvationTimeout.ToString());
            return true;
        }

        [NullableContext(2)]
        [CompilerGenerated]
        public static bool operator !=(DispatcherOptions left, DispatcherOptions right)
        {
            return !(left == right);
        }

        [NullableContext(2)]
        [CompilerGenerated]
        public static bool operator ==(DispatcherOptions left, DispatcherOptions right)
        {
            if ((object)left != right)
            {
                if ((object)left != null)
                {
                    return left.Equals(right);
                }
                return false;
            }
            return true;
        }

        [CompilerGenerated]
        public override int GetHashCode()
        {
            return EqualityComparer<Type>.Default.GetHashCode(EqualityContract) * -1521134295 + EqualityComparer<TimeSpan>.Default.GetHashCode(<InputStarvationTimeout>k__BackingField);
        }

        [NullableContext(2)]
        [CompilerGenerated]
        public override bool Equals(object obj)
        {
            return Equals(obj as DispatcherOptions);
        }

        [NullableContext(2)]
        [CompilerGenerated]
        public virtual bool Equals(DispatcherOptions other)
        {
            if ((object)this != other)
            {
                if ((object)other != null && EqualityContract == other.EqualityContract)
                {
                    return EqualityComparer<TimeSpan>.Default.Equals(<InputStarvationTimeout>k__BackingField, other.<InputStarvationTimeout>k__BackingField);
                }
                return false;
            }
            return true;
        }

        [CompilerGenerated]
        public virtual DispatcherOptions <Clone>$()
        {
            return new DispatcherOptions(this);
        }

        [CompilerGenerated]
        protected DispatcherOptions(DispatcherOptions original)
        {
            <InputStarvationTimeout>k__BackingField = original.<InputStarvationTimeout>k__BackingField;
        }

        public DispatcherOptions()
        {
            <InputStarvationTimeout>k__BackingField = TimeSpan.FromSeconds(1.0);
            base..ctor();
        }
    }

/// need to be lowered on resource-constrained platforms where input events are processed on
/// the same thread as rendering.
/// </remarks>
public TimeSpan InputStarvationTimeout { get; set; } = TimeSpan.FromSeconds(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public TimeSpan InputStarvationTimeout { get; set; } = TimeSpan.FromSeconds(1);
public TimeSpan InputStarvationTimeout { get; init; } = TimeSpan.FromSeconds(1);

Copy link
Member Author

Choose a reason for hiding this comment

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

None of our other *Options classes uses init (because they predate init properties). Not sure if it's better to make new options properties init-only and leave the others, or convert them all for 12.0. Not very important IMO though in the sense that init won't solve any real-world problem here.

Copy link
Contributor

@Gillibald Gillibald Mar 29, 2024

Choose a reason for hiding this comment

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

As long as these options are only read once by the backend during initialization they can be mutable.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046706-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 added the backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch label Apr 20, 2024
@maxkatz6 maxkatz6 merged commit 8a65cb6 into master Apr 20, 2024
11 checks passed
@maxkatz6 maxkatz6 deleted the fixes/input-starvation branch April 20, 2024 00:29
maxkatz6 pushed a commit that referenced this pull request Apr 20, 2024
* Use 0.2 seconds for input starvation cutoff.

1 second was too long when running on resource-constrained devices.

* Allow input starvation code to run.

Using `ScheduleRender(true)` here prevented the input starvation code from running.

* Add DispatcherOptions.

And allow setting the input starvation timeout there.
@maxkatz6 maxkatz6 added backported-11.1.x and removed backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch labels Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants