Status Update
Comments
ti...@google.com <ti...@google.com>
ib...@google.com <ib...@google.com> #2
Project: platform/frameworks/support
Branch: androidx-main
Author: Chris Craik <
Link:
Reimplement BenchmarkRule / BenchmarkState on top of coroutines
Expand for full commit details
Reimplement BenchmarkRule / BenchmarkState on top of coroutines
Test: benchmark-common, benchmark-junit4
Bug: 389149423
Fixes: 311242861
This refactor achieves several important goals:
- Move from the extremely complex stateful nature of BenchmarkState to layered suspending functions (the old state deferral system was very obtuse and brittle)
- Simplify the existing measureRepeatedOnMainThread yield behavior to use actual yield()
- Rewrite the thermal throttling sleep to yield() instead of sleep() (which works around ANRs when using measureRepeatedOnMainThread)
- Provide a clear config -> execution -> output control flow, including a clear top level measureRepeated function (which declares all its inputs, and almost wires up the outputs)
- Move as much logic as possible from the JUnit4 layer to benchmark-common
These will indirectly make it more possible to:
- Run trace post-processing within microbenchmark (e.g. interference detection and retry)
- More cleanly add configuration in MicrobenchmarkConfig (similar to what warmupCount / repeatCount BenchmarkState constructor params were previously trying to offer)
- Move benchmark-common to KMP if desired
- Write suspending variants of the benchmark APIs
The downsides are:
- Benchmarks with the `BenchmarkRule.getState().keepRunning()` pattern will no longer capture Perfetto traces
- In a follow up, a replacement API will be offered for Java that more closely matches kotlin
- BenchmarkState is implemented on top of a complex and custom coroutine logic mimicking a suspending sequence
- Ideally this implementation will be deprecated and not have to change much in the future
- Several experimental BenchmarkState APIs were removed, to make way for the new way of solving these problems (configuring benchmarks)
- The most useful of these moved to MicrobenchmarkConfig
The old BenchmarkRule/BenchmarkState are temporarily moved to internal
BenchmarkRuleLegacy/BenchmarkStateLegacy to enable easy internal
comparisons in the short term. Eventually these will be removed.
Relnote: "Refactored BenchmarkRule to be built on top of coroutines, and support better yield() behavior. This refactor removed several experimental BenchmarkState APIs, but will be followed by replacements as needed. Additionally, added runWithMeasurementDisabled to clarify behavior (all measurement is paused). In the future, runWithTimingDisabled will be deprecated."
Change-Id: I19837fc604b9e308957fc01ed009c0e921b8fe28
Files:
- M
benchmark/benchmark-common/api/current.txt
- M
benchmark/benchmark-common/api/restricted_current.txt
- M
benchmark/benchmark-common/build.gradle
- M
benchmark/benchmark-common/src/androidTest/java/androidx/benchmark/BenchmarkStateLegacyConfigTest.kt
- M
benchmark/benchmark-common/src/androidTest/java/androidx/benchmark/BenchmarkStateLegacyTest.kt
- M
benchmark/benchmark-common/src/androidTest/java/androidx/benchmark/MicrobenchmarkPhaseConfigTest.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/Arguments.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/BenchmarkState.kt
- A
benchmark/benchmark-common/src/main/java/androidx/benchmark/BenchmarkStateLegacy.kt
- A
benchmark/benchmark-common/src/main/java/androidx/benchmark/Microbenchmark.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/MicrobenchmarkConfig.kt
- A
benchmark/benchmark-common/src/main/java/androidx/benchmark/MicrobenchmarkOutput.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/MicrobenchmarkPhase.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/Profiler.kt
- A
benchmark/benchmark-common/src/main/java/androidx/benchmark/TestDefinition.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/ThrottleDetector.kt
- M
benchmark/benchmark-common/src/main/java/androidx/benchmark/perfetto/PerfettoCaptureWrapper.kt
- M
benchmark/benchmark-junit4/api/current.txt
- M
benchmark/benchmark-junit4/api/restricted_current.txt
- M
benchmark/benchmark-junit4/src/androidTest/java/androidx/benchmark/junit4/BenchmarkRuleAnnotationTest.kt
- A
benchmark/benchmark-junit4/src/androidTest/java/androidx/benchmark/junit4/BenchmarkRuleLegacyTest.kt
- M
benchmark/benchmark-junit4/src/androidTest/java/androidx/benchmark/junit4/BenchmarkRuleTest.kt
- M
benchmark/benchmark-junit4/src/main/java/androidx/benchmark/junit4/BenchmarkRule.kt
- A
benchmark/benchmark-junit4/src/main/java/androidx/benchmark/junit4/BenchmarkRuleLegacy.kt
- M
benchmark/benchmark/src/androidTest/java/androidx/benchmark/benchmark/BenchmarkStateBenchmark.kt
- A
benchmark/benchmark/src/androidTest/java/androidx/benchmark/benchmark/BenchmarkStateLegacyBenchmark.kt
- A
benchmark/benchmark/src/androidTest/java/androidx/benchmark/benchmark/MeasureRepeatedSampleBenchmark.kt
- A
benchmark/benchmark/src/androidTest/java/androidx/benchmark/benchmark/TrivialKotlinBenchmarkLegacy.kt
- M
compose/runtime/runtime/compose-runtime-benchmark/src/androidTest/java/androidx/compose/runtime/benchmark/ComposeBenchmarkBase.kt
Hash: 329bd0d25247f1eb6c683b921b8cd06f60bf3ad2
Date: Fri Jul 12 15:09:22 2024
ja...@gmail.com <ja...@gmail.com> #3
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.benchmark:benchmark-common:1.4.0-alpha07
androidx.benchmark:benchmark-junit4:1.4.0-alpha07
ib...@google.com <ib...@google.com> #4
Thanks - it looks like the original image has no Exif data, so something is going wrong with the way ExifInterface
is adding Exif data. We have
ib...@google.com <ib...@google.com> #5
Dropping 001.webp
into the existing testWebpWithoutExif()
test causes it to fail, so there's definitely something different about this file.
I took a look at 001.webp
in a hex editor and noticed that its declared file length (from the RIFF
header) is 0xD4C00100
, which (because it's little endian) is 114900 in decimal. This should be the length of the file from offset 8 (i.e. file should be 8 bytes longer than this) - but the file is actually 12 bytes longer than this (114912) with 4 trailing 0x04
bytes on the end.
Removing these trailing bytes and re-running the test causes it to pass.
Inspecting 001_corrupt.webp
I can see the trailing 0x04
bytes at the end.
So what happens when we write out a WebP file is:
- We add the
RIFF
header and placeholder for the file length - We synthesise a VP8X header.
- We copy the existing VP8 chunk
- Append the new Exif chunk
- Copy the rest of the file (the 4 trailing
0x04
bytes). - Go back to the beginning and fill in the file length based on how much data we wrote in steps 2, 3, 4 & 5.
- This ends up including the trailing
0x04
bytes in the RIFF length, but they're not a valid chunk - so the file ends up being invalid.
- This ends up including the trailing
The issue comes from copying the whole of the input file, and assuming it should all be covered by the new RIFF header, instead of considering the RIFF length of the original file. I will send a change for review which only includes existing RIFF data and new Exif data in the new RIFF length, and then copies over any additional trailing data outside the RIFF structure.
ib...@google.com <ib...@google.com>
ja...@gmail.com <ja...@gmail.com> #6
ap...@google.com <ap...@google.com> #7
Project: platform/frameworks/support
Branch: androidx-main
Author: Ian Baker <
Link:
Exclude trailing non-WebP data from length when writing new file
Expand for full commit details
Exclude trailing non-WebP data from length when writing new file
Some files have trailing data outside the length defined by the RIFF
header. ExifInterface previously naively copied this data and then
calculated a new RIFF length which included it, resulting in an invalid
file.
This change only copies RIFF-defined data from the old file before
calculating the new RIFF length, and then appends any additional data
after that - in order to more closely mimic the input file's structure
without throwing away the trailing data.
Test: ExifInterfaceTest
Bug: 385766064
Change-Id: I8a955cf85d229ad4af5a1e02aa891cfca0e54d60
Files:
- M
exifinterface/exifinterface/src/androidTest/java/androidx/exifinterface/media/ExifInterfaceTest.java
- A
exifinterface/exifinterface/src/androidTest/res/raw/webp_without_exif_trailing_data.webp
- M
exifinterface/exifinterface/src/main/java/androidx/exifinterface/media/ExifInterface.java
Hash: d21234605366120bb1af4ea1109e15b6c23d93f0
Date: Fri Jan 03 11:30:32 2025
pr...@google.com <pr...@google.com> #8
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.exifinterface:exifinterface:1.4.0-beta01
Description
Version used: 1.4.0-alpha01
Devices/Android versions reproduced on: Pixel 8a
After calling saveAttributes on an webp image, the image can not be viewed anymore on an Android device. Windows can for some reason still display the image with the inbuilt Photos program and e.g.