cashapp / paparazzi

Render your Android screens without a physical device or emulator
https://cashapp.github.io/paparazzi/
Apache License 2.0
2.32k stars 216 forks source link

Different solid color rectangles of the same area have the same screenshots #1527

Closed And42 closed 1 month ago

And42 commented 3 months ago

Description If you have multiple different components that are solid rectangles of the same color and they have the same pixel area, library will produce only one screenshot for all of them

Steps to Reproduce Project: https://github.com/And42/PaparazziSameContentIssue 3 different components: https://github.com/And42/PaparazziSameContentIssue/blob/12a4d4475a4a129def10bfcf3509aa996e5023b5/app/src/main/java/com/example/paparazzisamecontentissue/MyNiceComponent.kt Recorded screenshots: https://github.com/And42/PaparazziSameContentIssue/tree/12a4d4475a4a129def10bfcf3509aa996e5023b5/app/src/test/snapshots/images

Expected behavior Library should produce different screenshots for the components

Additional information:

Screenshots Expected: com example paparazzisamecontentissue_PaparazziTest_testMyNiceVerticalLineComponent com example paparazzisamecontentissue_PaparazziTest_testMyNiceHorizontalLineComponent com example paparazzisamecontentissue_PaparazziTest_testMyNiceSquareComponent

Actual: com example paparazzisamecontentissue_PaparazziTest_testMyNiceVerticalLineComponent com example paparazzisamecontentissue_PaparazziTest_testMyNiceHorizontalLineComponent com example paparazzisamecontentissue_PaparazziTest_testMyNiceSquareComponent

Possible fix: I've already found that this is most probably caused by the function that calculates the hash of the image: https://github.com/cashapp/paparazzi/blob/8912b77cbefcb422b2697d96566b5e7c50150d35/paparazzi/src/main/java/app/cash/paparazzi/HtmlReportWriter.kt#L121 The current code just writes the contents of the image, which leads to images with the same content having the same ids. A simple fix is to add

sink.writeInt(image.width)
sink.writeInt(image.height)

before adding the image contents

BrianGardnerAtl commented 3 months ago

hey, I tested out your sample project but was unable to reproduce your findings. Upon running the record command both the horizontal line and square components updated to the expected shapes. I am running on an M3 max mac machine and I don't currently have a windows machine to test this on. I'll check with my teammates to see if they have an environment we can use to test.

And42 commented 3 months ago

Hey!

Hmm. Initially I got the issue on the Mac machine with M1 Max. It's just that that is my work machine and I created the repro project on my personal machine with windows. So, it shouldn't be an OS specific issue.

I'll look a bit more into how I record the snapshots and your commit, hopefully, will be able to find when the issue happens. One question though, how did you record the snapshots in the referenced commit? Did you record one by one or the whole set of them by running recording on the whole class?

It's just that I noticed that the paparazzi code looks at the build folder reports for the existing hashed snapshots. When I run for the whole class, paparazzi reuses the image created by another method with the same hash.

By the way, the issue for me only happens on the recording stage. The verification works correctly every time. So, your test may not catch the problem

Update: checked out your branch with the test, ran gradle :test-projects:image-hash:recordPaparazziDebug from the Android Studio gradle dialog and reproduced the issue

Video: https://github.com/user-attachments/assets/b352fc88-0b8d-49fd-be2e-dec600323777

image-hash\build\reports\paparazzi\debug\images contain only one image instead of 3: image

BrianGardnerAtl commented 3 months ago

One question though, how did you record the snapshots in the referenced commit? Did you record one by one or the whole set of them by running recording on the whole class?

I recorded all of them using: :test-projects:image-hash:recordPaparazziDebug --tests "app.cash.paparazzi.plugin.test.ImageHashTest"

I got my windows machine set up this morning and I was able to reproduce the failure in your sample project so that should help shed some light on the issue

BrianGardnerAtl commented 3 months ago

I've updated the test project to always record the snapshots before verifying them and now the horizontal and square components are failing just like yours. Now with the failing test case I can update the hash function to avoid these types of collisions.

And42 commented 3 months ago

Awesome, thank you!

jrodbx commented 3 months ago

I got my windows machine set up this morning and I was able to reproduce the failure in your sample project so that should help shed some light on the issue

I'm curious: what OS specific change is causing this issue? HashingSink.sha1? HashingSink.hash? ByteString.hex?

I ask because given a cursory look at the code, I would expect the OP's issue (which makes sense!) to fail on all OSses, so I'd love us to dig in more, especially if we can repro on a Windows machine now.

Regarding adding width/height to the hash, it may solve this bug, but won't solve the overall issue. Here's an example:

@Composable
@Preview
fun Checkerboard() {
    val tileSize = 8.dp.toPx()
    val image = ImageBitmap(tileSize.roundToInt() * 2, tileSize.roundToInt() * 2)
    val canvas = androidx.compose.ui.graphics.Canvas(image)
    val fill = Paint().also { it.style = PaintingStyle.Fill; it.color = Color(0x22000000) }
    canvas.drawRect(0f, 0f, tileSize, tileSize, fill)
    canvas.drawRect(tileSize, tileSize, tileSize * 2, tileSize * 2, fill)
    val brush = ShaderBrush(ImageShader(image, TileMode.Repeated, TileMode.Repeated))
    Canvas(
        modifier = Modifier
            .background(Color.White)
            .size(16.dp),
        onDraw = { drawRect(brush) }
    )
}

@Composable
@Preview
fun CheckerboardInverted() {
    val tileSize = 8.dp.toPx()
    val image = ImageBitmap(tileSize.roundToInt() * 2, tileSize.roundToInt() * 2)
    val canvas = androidx.compose.ui.graphics.Canvas(image)
    val fill = Paint().also { it.style = PaintingStyle.Fill; it.color = Color(0x22000000) }
    canvas.drawRect(tileSize, 0f, tileSize * 2, tileSize, fill)
    canvas.drawRect(0f, tileSize, tileSize, tileSize * 2, fill)
    val brush = ShaderBrush(ImageShader(image, TileMode.Repeated, TileMode.Repeated))
    Canvas(
        modifier = Modifier
            .background(Color.White)
            .size(16.dp),
        onDraw = { drawRect(brush) }
    )
}
Screenshot 2024-08-09 at 11 16 33 AM
BrianGardnerAtl commented 3 months ago

Let me check and see how the hashes compare on windows and mac. From there I can see how each are calculated and determine if there's a way to avoid the difference

BrianGardnerAtl commented 3 months ago

Ok so far I've found that the hashes for the images on both platforms are all the same for all the images, which makes sense. The tests pass on Mac but I noticed that if you open the html report all the images are whatever the last recorded one is, which makes sense because the snapshot json uses the report images which use the image hash for the filename.

A couple differences I've found so far:

  1. The image saved in the report folder is different between windows and mac, even though the hash is the same. On windows the image is the vertical line while on mac the image is the square. Both platforms are running the tests in the same order, meaning that windows is saving the vertical line, which is the first snapshot taken, and is not overwriting it with the other snapshots. Mac is saving the square, which is the last snapshot, meaning that it is overwriting the images. This makes sense because the recorded images on Mac are all correct, meaning they've been correctly copied from the report directory.
  2. The only other difference I've found relates to the snapshotTmpFile used within the close() function of the FrameHandler in HtmlReportWriter. I was curious so I logged the output of both the rename and delete operations on that file and got opposite results on each platform. Mac successfully renamed the file but the delete command failed. Windows failed to rename the file but was successful at deleting it.

My gut feeling is that whatever is causing the discrepancy with the temp file is the cause of the issues here. Resolving it should fix the snapshot recordings on Windows. However, we will still need to update the hash function because the HTML report uses the hash files. This makes it look like all of the tests output the same view.

I'm going to keep digging on this but I've got daycare pickup so it may be a bit before I get back to it.

And42 commented 3 months ago

@jrodbx Just a note. The compose elements you've mentioned produce different hashes in the current implementation, so they shouldn't be a problem there: image

Hashing function result in the code depends not only on the contents of the image but also on the time when this content is added to hash. If we simplify your example into 4 pixels, we will get [[Grey, White], [White, Grey]] for checkerboard and [[White, Grey], [Grey, White]] for the inverted checkboard. And although the contents are the same (2 greys and 2 whites), the order of them is different, which will cause hash to be different.

Solid colors there have the collision now because they have both same contents (same color always) and same order (same pixels always have the same order)

jrodbx commented 3 months ago

@jrodbx Just a note. The compose elements you've mentioned produce different hashes in the current implementation, so they shouldn't be a problem there: image

Hashing function result in the code depends not only on the contents of the image but also on the time when this content is added to hash. If we simplify your example into 4 pixels, we will get [[Grey, White], [White, Grey]] for checkerboard and [[White, Grey], [Grey, White]] for the inverted checkboard. And although the contents are the same (2 greys and 2 whites), the order of them is different, which will cause hash to be different.

Solid colors there have the collision now because they have both same contents (same color always) and same order (same pixels always have the same order)

Odd, the hashes did not compute differently on my machine. Could that be the root issue?

but also on the time when this content is added to hash

can you elaborate more on this? where does the time factor in?

And42 commented 3 months ago

Odd, the hashes did not compute differently on my machine. Could that be the root issue?

Oh, wow, if that actually happens for you then it can be an issue for sure. Although, if it happens then it may mean that the order doesn't matter for hashing and if that is the case, then even your fix with putting coordinates before values may not work

can you elaborate more on this? where does the time factor in?

By time I meant the order. Hashing algorithms like md5 or sha produce different outputs for different inputs (except collisions, but they are relatively rare). So, the hash of [Gray, White, White, Gray] will be different from [White, Gray, Gray, White] because those color arrays result in different input byte arrays and after those are hashed they should usually be different too. The function in the current code uses sha-1 for hashing: https://github.com/cashapp/paparazzi/blob/8912b77cbefcb422b2697d96566b5e7c50150d35/paparazzi/src/main/java/app/cash/paparazzi/HtmlReportWriter.kt#L121 I didn't check it locally (so, after the previous findings I'm not 100% sure anymore, heh), but I assume it should also produce different results just because of the underlying hashing algorithm

jrodbx commented 1 month ago

Closed by #1535 and #1556