From 5c53492eba67819b5bdcab881a4d2ab887d01cd0 Mon Sep 17 00:00:00 2001 From: Johann Beleites Date: Mon, 19 Feb 2024 13:32:56 +0100 Subject: [PATCH] SONARJAVA-4841 Add S6898: Avoid high frame rates on mobile (#4652) --- .../src/main/java/android/view/Surface.java | 6 ++ .../java/android/view/SurfaceControl.java | 14 +++++ ...voidHighFrameratesOnMobileCheckSample.java | 30 ++++++++++ .../AvoidHighFrameratesOnMobileCheck.java | 56 ++++++++++++++++++ .../AvoidHighFrameratesOnMobileCheckTest.java | 44 ++++++++++++++ .../org/sonar/plugins/java/CheckList.java | 2 + .../org/sonar/l10n/java/rules/java/S6898.html | 58 +++++++++++++++++++ .../org/sonar/l10n/java/rules/java/S6898.json | 24 ++++++++ 8 files changed, 234 insertions(+) create mode 100644 java-checks-test-sources/default/src/main/java/android/view/SurfaceControl.java create mode 100644 java-checks-test-sources/default/src/main/java/checks/AvoidHighFrameratesOnMobileCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheckTest.java create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.html create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.json diff --git a/java-checks-test-sources/default/src/main/java/android/view/Surface.java b/java-checks-test-sources/default/src/main/java/android/view/Surface.java index b41dd2ace8d..f3953a02d32 100644 --- a/java-checks-test-sources/default/src/main/java/android/view/Surface.java +++ b/java-checks-test-sources/default/src/main/java/android/view/Surface.java @@ -24,4 +24,10 @@ public class Surface { public void release() { // mock implementation } + + public void setFrameRate(float frameRate, int compatibility, int changeFrameRateStrategy) { + } + + public void setFrameRate(float frameRate, int compatibility) { + } } diff --git a/java-checks-test-sources/default/src/main/java/android/view/SurfaceControl.java b/java-checks-test-sources/default/src/main/java/android/view/SurfaceControl.java new file mode 100644 index 00000000000..9b1c2c20956 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/android/view/SurfaceControl.java @@ -0,0 +1,14 @@ +package android.view; + +public class SurfaceControl { + public SurfaceControl.Transaction setFrameRate(SurfaceControl sc, float frameRate, int compatibility) { + return new Transaction(); + } + + public SurfaceControl.Transaction setFrameRate(SurfaceControl sc, float frameRate, int compatibility, int changeFrameRateStrategy) { + return new Transaction(); + } + + public static class Transaction { + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/AvoidHighFrameratesOnMobileCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/AvoidHighFrameratesOnMobileCheckSample.java new file mode 100644 index 00000000000..e404e058cc5 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/AvoidHighFrameratesOnMobileCheckSample.java @@ -0,0 +1,30 @@ +package checks; + +import android.view.Surface; +import android.view.SurfaceControl; + +public class AvoidHighFrameratesOnMobileCheckSample { + + private static final int HIGH_THRESHOLD = 120; + private int HIGH_THRESHOLD_NON_FINAL = 120; + + void noncompliant(Surface surface, SurfaceControl surfaceControl) { + surface.setFrameRate(120, 0, 0); // Noncompliant [[sc=26;ec=29]] {{Avoid setting high frame rates higher than 60 on mobile devices.}} + surface.setFrameRate(120, 0); // Noncompliant + surfaceControl.setFrameRate(surfaceControl, 120, 0, 0); // Noncompliant + surfaceControl.setFrameRate(surfaceControl, 120, 0); // Noncompliant + + surface.setFrameRate(61, 0, 0); // Noncompliant + surface.setFrameRate(HIGH_THRESHOLD, 0); // Noncompliant + } + + void compliant(Surface surface, SurfaceControl surfaceControl) { + surface.setFrameRate(60, 0, 0); + surface.setFrameRate(60, 0); + surfaceControl.setFrameRate(surfaceControl, 60, 0, 0); + surfaceControl.setFrameRate(surfaceControl, 60, 0); + + surface.setFrameRate(1, 0, 0); + surface.setFrameRate(HIGH_THRESHOLD_NON_FINAL, 0); // Compliant FN, as we don't currently resolve non-final non-static fields. + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheck.java b/java-checks/src/main/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheck.java new file mode 100644 index 00000000000..a79aed4414e --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheck.java @@ -0,0 +1,56 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2024 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks; + +import java.util.List; +import java.util.Map; +import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S6898") +public class AvoidHighFrameratesOnMobileCheck extends IssuableSubscriptionVisitor { + + private static final int DEFAULT_THRESHOLD = 60; + private static final Map FRAME_RATE_SETTERS = Map.of( + MethodMatchers.create().ofTypes("android.view.Surface").names("setFrameRate").withAnyParameters().build(), 0, + MethodMatchers.create().ofTypes("android.view.SurfaceControl").names("setFrameRate").withAnyParameters().build(), 1); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.METHOD_INVOCATION); + } + + @Override + public void visitNode(Tree tree) { + var mit = (MethodInvocationTree) tree; + FRAME_RATE_SETTERS.entrySet().stream().filter(e -> e.getKey().matches(mit)).findFirst().ifPresent(e -> { + var frameRateArg = mit.arguments().get(e.getValue()); + var frameRateArgVal = ExpressionUtils.resolveAsConstant(frameRateArg); + + if (frameRateArgVal instanceof Number frameRateNumber && frameRateNumber.intValue() > DEFAULT_THRESHOLD) { + reportIssue(frameRateArg, "Avoid setting high frame rates higher than " + DEFAULT_THRESHOLD + " on mobile devices."); + } + }); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheckTest.java new file mode 100644 index 00000000000..d27b4808f4a --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/AvoidHighFrameratesOnMobileCheckTest.java @@ -0,0 +1,44 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2024 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; +import org.sonar.java.checks.verifier.TestUtils; + +class AvoidHighFrameratesOnMobileCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(TestUtils.mainCodeSourcesPath("checks/AvoidHighFrameratesOnMobileCheckSample.java")) + .withCheck(new AvoidHighFrameratesOnMobileCheck()) + .verifyIssues(); + } + + @Test + void test_no_semantics() { + CheckVerifier.newVerifier() + .onFile(TestUtils.mainCodeSourcesPath("checks/AvoidHighFrameratesOnMobileCheckSample.java")) + .withCheck(new AvoidHighFrameratesOnMobileCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java index 6c3c4009417..7940b91ec7e 100644 --- a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java +++ b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java @@ -44,6 +44,7 @@ import org.sonar.java.checks.AssertsOnParametersOfPublicMethodCheck; import org.sonar.java.checks.AssignmentInSubExpressionCheck; import org.sonar.java.checks.AtLeastOneConstructorCheck; +import org.sonar.java.checks.AvoidHighFrameratesOnMobileCheck; import org.sonar.java.checks.BasicAuthCheck; import org.sonar.java.checks.BigDecimalDoubleConstructorCheck; import org.sonar.java.checks.BluetoothLowPowerModeCheck; @@ -724,6 +725,7 @@ public final class CheckList { AuthorizationsStrongDecisionsCheck.class, AutowiredOnConstructorWhenMultipleConstructorsCheck.class, AutowiredOnMultipleConstructorsCheck.class, + AvoidHighFrameratesOnMobileCheck.class, AvoidQualifierOnBeanMethodsCheck.class, AwsConsumerBuilderUsageCheck.class, AwsCredentialsShouldBeSetExplicitlyCheck.class, diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.html new file mode 100644 index 00000000000..5eee58ee58e --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.html @@ -0,0 +1,58 @@ +

The Frame Rate API allows applications to communicate their desired frame rate to the Android platform to enhance the user +experience. The API is useful since many devices now offer varying refresh rates like 60Hz, 90Hz, or 120Hz.

+

Why is this an issue?

+

Standard applications don’t require a display refresh rate above 60Hz, hence it is advisable to avoid higher frequencies to avoid unnecessary +energy consumption.

+

The rule flags an issue when setFrameRate() is invoked with a frameRate higher than 60Hz for android.view.Surface and +android.view.SurfaceControl.Transaction.

+

It’s important to note that the scheduler considers several factors when determining the display refresh rate. Therefore, using +setFrameRate() doesn’t guarantee your app will achieve the requested frame rate.

+

What is the potential impact?

+
    +
  • Usability: the device may run out of battery faster than expected.
    • +
  • Sustainability: the extra battery usage has a negative impact on the environment.
    • +
+

How to fix it

+

Use a frame rate of maximum 60Hz, unless you have a strong reason to used higher rates. Valid exceptions are gaming apps, especially those +with fast-paced action or high-quality graphics, or AR/VR apps.

+

Code examples

+

Noncompliant code example

+
+public class MainActivity extends AppCompatActivity {
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        setContentView(R.layout.activity_main);
+
+        SurfaceView surfaceView = findViewById(R.id.my_surface_view);
+        Surface surface = surfaceView.getHolder().getSurface();
+
+        surface.setFrameRate(90.0f, Surface.FRAME_RATE_COMPATIBILITY_FIXED_SOURCE); // Noncompliant
+    }
+}
+
+

Compliant solution

+
+public class MainActivity extends AppCompatActivity {
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        setContentView(R.layout.activity_main);
+
+        SurfaceView surfaceView = findViewById(R.id.my_surface_view);
+        Surface surface = surfaceView.getHolder().getSurface();
+
+        surface.setFrameRate(60.0f, Surface.FRAME_RATE_COMPATIBILITY_FIXED_SOURCE); // Compliant
+    }
+}
+
+

Resources

+

Documentation

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.json new file mode 100644 index 00000000000..920f18f6a26 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6898.json @@ -0,0 +1,24 @@ +{ + "title": "High frame rates should not be used", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "android", + "sustainability" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6898", + "sqKey": "S6898", + "scope": "Main", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW" + }, + "attribute": "EFFICIENT" + } +}