IssueTracker

Ideally there would be two thresholds we could customize:

1. Pixel difference threshold, i.e how much any given pixel can change before it's considered different from the target pixel.
2. Total difference threshold, i.e how much of the total screenshot can be different from the target

In my situation, the ideal settings (and what I currently use on other tools/platforms) is ~1% pixel difference threshold to ignore text rendering and anti-aliasing differences that occur across hardware and a 0% total difference threshold so that any change more significant than that is flagged immediately.

Using a single total difference threshold to ignore aliasing is unideal, because on screenshots with text this value can end up being very large to account for every possible pixel that could receive different aliasing. So large that other actually important differences may sneak by undetected, e.g. the height or color of a divider line.

I like the idea of having multiple thresholds. Could you please explain option 2 (total diff) in more detail?

Option 2 (total difference) is the percentage of the image that is allowed to be 'different' from the reference image. For example, if the reference image is 10x10 pixels, a 5% threshold would allow the test to pass even if up to 5 pixels do not match the reference. In my opinion, this is the least useful threshold option, even though most other screenshot testing tools have it, as it's the most straightforward threshold to implement. I avoid using this kind of threshold because it doesn't scale well with large screenshots. If I'm taking a screenshot of an entire screen of my app, say 1080x1920, even a small threshold of 0.5% allows for over 10,000 pixels to not match the reference. This means that it's possible the test won't catch important changes like the color of a border, a change of font, or potentially even a small icon disappearing.

What I find to be a more useful option is configuring when any given pixel is considered different from the reference pixel. For example, if I have a screenshot of an black circle on a white background, my laptop (an Apple Silicon MacBook) will anti-alias the image slightly differently than a colleague's Windows desktop or the Linux instance running my tests for CI. Even though to the human eye the image is identical, it will fail due to the minor differences in the gray pixels used to anti-alias the edge of the circle. In this situation, I don't care if a pixel that was #9A9A9A is now #9B9B9B; they're essentially identical, and I'd want the test to ignore that difference.

Trying to work around this kind of issue with option 2 isn't ideal. Larger screenshots have more anti-aliasing, which necessitates a larger total difference threshold to ignore the larger number of imperceptible differences, which in turn just increases the potential for a meaningful change to not be detected.

seems someone is working on this check out this commit. Tried it in alpha-05 but didn't work so I suspect it will be available in alpha-06.

For configuring check the ScreenShotTest.kt file

Diff threshold feature is enabled since alpha06 version. This does not solve the platform related rendering differences. Tracking that as a separate issue and closing this

So it's that commit that increase the size but it's the reason I suspected:

kotlin.Metadata
|- is reflected from:
|  void com.squareup.moshi.internal.Util.<clinit>()
|- is reachable from:
|  com.squareup.moshi.internal.Util
|- is referenced from:
|  com.squareup.moshi.JsonAdapter com.squareup.moshi.Moshi.adapter(java.lang.reflect.Type,java.util.Set,java.lang.String)
|- is invoked from:
|  void com.genimee.android.yatse.mediacenters.emby.api.model.Models_PlaybackStopInfoJsonAdapter.<init>(com.squareup.moshi.Moshi)
|- is referenced in keep rule:
|  D:\_Gradle\Yatse\emby\build\intermediates\consumer_proguard_dir\release\lib0\proguard.txt:173:1
|- is satisfied with precondition:
|  com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo
|- is referenced from:
|  java.lang.String com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo.LiveStreamId

The referenced keep rule:

-if class com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo
-keepnames class com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo
-if class com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo
-keep class com.genimee.android.yatse.mediacenters.emby.api.model.Models_PlaybackStopInfoJsonAdapter {
    public <init>(com.squareup.moshi.Moshi);
}
-keepnames class kotlin.jvm.internal.DefaultConstructorMarker
-keepclassmembers class com.genimee.android.yatse.mediacenters.emby.api.model.Models$PlaybackStopInfo {
    public synthetic <init>(java.lang.String,java.lang.String,java.lang.String,int,kotlin.jvm.internal.DefaultConstructorMarker);
    public <init>(java.lang.String,java.lang.String,java.lang.String,int,kotlin.jvm.internal.DefaultConstructorMarker);
}

Maybe it's not to late to address https://issuetracker.google.com/issues/152811099 as it can bite more people in more cases

Without discussing b/152811099 here, -keeping Kotlin.Metadata should not be kept it for all classes that ends up in the output - only for things that are directly kept. That thing only accounts for the majority. I will keep this bug open until that part is fixed, since we have b/152811099.

Not sure to follow you.

The keep rule for Metadata is autogenerated from R8 because of the reflection call, since that call does not include any class related data, it can only generate a rule that keep all, before applying my hacky rule, it was the case will all R8 versions.

Since the other issue is feature request P3 I have little hope about it for 2.1 :)

What would be the proper way to drop that reflection call before it generate the rule, since my hacky way no more works, I suppose there's another proper way? (The workaround in that issue probably works, but it requires to use a custom Moshi version that most won't and merging on that project seems complicated those days)

Kotlin.Metadata is an annotation and the current behavior in R8 is that if a keep rule is generated/written for an annotation, then it will be kept on all classes that ends up in the output. However, kotlin.Metadata is special, and keeping it for all classes seems like something most people would not need.

We therefore would like to interpret Kotlin.Metadata more as an attribute and only keep that annotation on classes that has a specific keep-rule.

What is fun about https://r8-review.googlesource.com/48200 is that it actually does the opposite. Before that change, we would discard kotlin metadata for renamed classes, since we did not know how to rewrite in the metadata anyway. With that CL, we now keep the metadata for all classes in the output.

That is also why I think you see the majority of the size increase from that CL. Before, we would still keep Kotlin.Metadata, but only on the classes that was kept additionally. After, we keep it on all classes in the output. So, without even discussing b/152811099, that CL is not what we want and we should change the behavior.

Regarding the reflection call and keep, b/152811099 should be the place to discuss that.

About https://issuetracker.google.com/issues/152811099 this is a generic request to have a way to indicate to R8 that a reflection call should not be taken in account to auto generate rules, this is a self contained need not really related to the issue.

I believe you and understand the change in what is kept from Metadata and I can open another issue if you want but

kotlin.Metadata
|- is reflected from:
|  void com.squareup.moshi.internal.Util.<clinit>()

Should not exist because of

-assumevalues public class com.squareup.moshi.internal.Util {
  private static final * METADATA return null;
}

On R8 2.x without this rule metadata where kept and I did have the

kotlin.Metadata
|- is reflected from:
|  void com.squareup.moshi.internal.Util.<clinit>()

With this rule only metadata with specific rules in R8 configuration where kept. (I can trigger a build and give the result from whyareyoukeeping if needed) So all worked as intended.

So without taking in account the Metadata change and everything the result from -whyareyoukeeping class kotlin.Metadata have changed and this is the part that bothers me and I do not understand and will probably still keep unneeded stuff even if you fix the too broad keep of that specific commit.

Ok, I see a few different questions discussed here:

1. How to turn off generated keep rules for Clazz.forName(...) (b/152811099)

   1.1 Can we turn it off by using -assumevalues? (b/152811099)

2. Should we even consider Clazz.forName("kotlin.Metadata") to be equal to -keep class kotlin.Metadata since -keep class some.annotation will force that we keep that annotation where present on all output. Most users will use kotlin reflect anyway to look at the information, since it is encoded and not directly available (Internal tracking bug)

3. Should R8 treat kotlin.Metadata as an annotation since the penalty is pretty big and most users will not need to reflect on it. And if they do, then they should have a keep rule for the item that they would like to have kotlin.Metadata present on.

With this rule only metadata with specific rules in R8 configuration where kept. (I can trigger a build and give the result from whyareyoukeeping if needed) So all worked as intended.

I may be reading it wrong, but are you not saying that -whyareyoukeeping information is identical between the two, and the difference is that kotlin.Metadata is now present on many more classes? If so, we agree fully. Before the CL i point to, that was the behavior. After it is not. The main difference between 2.1.4-dev and 2.1.5-dev is that CL, because we would discard all meta-data for classes that are renamed (that is, all classes without keep,allowobfuscation rules). If you find that -whyareyoukeeping is printing differently between the versions on the same -keep rules, please create another issue such that we can figure out if this is intentional.

At least, I am fixing 2. and 3. and will report back when that is done on this bug. All other things related to keep rules are orthogonal and should be handled in b/152811099.

Yes whyareyoukeeping result are different and where even more different before other changes I made, unfortunately I've made too many changes to too many variables to have meaningful data :(

I opened https://issuetracker.google.com/issues/154434597 for reference because currently nothing makes sense on both R8 version. Will complete when I get more actionable data, unless there's something obvious already.

tolriq@, could you please check if the size regression from Kotlin metadata annotations is still present in R8 2.1.31? Thanks.

I now use a patched moshi that no more keep metadata so can't really reproduce this issue anymore :(

But I've quickly tested 2.1.30 and there's no regression and a nice dex size reduction from 2.0.75

Is the release process changed?

2.1.31 is not found at standard place : http://storage.googleapis.com/r8-releases/raw/com/android/tools/r8/2.1.31/r8-2.1.31.pom ->

<Error>
<Code>NoSuchKey</Code>
<Message>The specified key does not exist.</Message>
<Details>No such object: r8-releases/raw/com/android/tools/r8/2.1.31/r8-2.1.31.pom</Details>
</Error>

All further releases are properly working, there was a glitch with that one and 2.0.76

Thanks for checking. The release process is the same, but there was an issue with 2.0.76 and 2.1.31 as you also found.

I will close this issue as the problem should be fixed. In particular, R8 should no longer preserve Kotlin metadata if kotlin.Metadata is not kept.

Please report back if you run into similar regressions in the future.