core: Fix wrong `blend_over()` - but with failing tests. #11038

iwannabethedev · 2023-05-13T21:02:49Z

This change seemingly fixes the new bug related to the SWF in #10954 (the issue in #10954 describes a previous, already fixed bug, the new bug has not been described in any issue, but described and investigated on Discord). This new bug came to surface with the latest nightly (2023-05-13), for the SWF in #10954 , where the bug causes flashing with crazy colors once the game is started from the game's main menu (the bug was expressed and came to surface, but not introduced as far as I can tell, with the changes in #11006 ). This PR is not regressing neither the panic bug nor the visual issue for #10955 and #11007 .

Superseded by: #11044 .

The apparent cause of the bug is #2488 , made some two years ago. Ruffle was much less mature back then (the code likely changed much more and unit tests were likely less cost-efficient overall development-wise), and it in my opinion makes sense that there were fewer unit tests back then for new code.

I believe that the primary cause of the bug was a lack of unit tests, which is understandable given that Ruffle was much newer back then. I suspect that a secondary cause of the bug was all the type-casts, which made the code more obscure and difficult to comprehend. I have made the code clearer, at the cost of making it more verbose, which I think is justified in this case.

~~For some reason, two tests are failing, so all the automatically run tests will presumably fail. I do not understand or know why they fail, I will need help from you to figure that out.~~

~~There are two TODOs in the code, which I would like to request reviewers to also consider.~~

Finally, I feel that it would be prudent for me to write some unit tests for blend_over(). Or at least create an issue, since it would be nice if this PR was figured out before the next nightly release. Though this fix is not that urgent, I believe, and it might be better to wait for the next nightly instead of hurrying.

iwannabethedev · 2023-05-13T21:06:32Z

CC @n0samu @adrian17 . (Also informing @Dinnerbone ).

Dinnerbone · 2023-05-13T23:38:01Z

Can you please split this into "refactor" + "change" commits? It's hard to tell what changed when you refactor at the same time

What exactly is the issue you fixed? Were there values that the blend was wrong for? If so can you add those to the test?

It's likely if some values were right and some wrong, and now a different set is right and wrong, then the method used to calculate the values is wrong. Floating point math may be more correct, or something akin to the unpremultiply LUT, but we have to check with tests.

iwannabethedev · 2023-05-13T23:48:05Z

As far as I can tell, the previous function implementation is obviously wrong, since it can result in values that are considerably larger than 255 despite being casted to u8. For instance:

source.red() + ((self.red() as u16 * (255 - sa as u16)) >> 8) as u8;

If source.red() can have the value 255, self.red() can have the value 255 and sa can have the value 0 or 1, then the result will be obviously larger than 255. Even though the result is casted to u8. This is consistent with the game Time4Cat having crazy colors - the colors likely overflowed in strange ways.

What I did is what in my experience is usually done with blending, namely where you take some proportion of one color, and take 1 - proportion from the other color.

Dinnerbone · 2023-05-14T00:11:02Z

As far as I can tell, the previous function implementation is obviously wrong, since it can result in values that are considerably larger than 255 despite being casted to u8. For instance:

Yep, good catch!

I'd still like to amend the test with new values, to prove this and guard against regressions.

We do all blending in premultiplied alpha, so the formula in normalized 0..1 values would be target = src + ((1 - src_alpha) * dst). If values aren't accurate to flash then we'll need to experiment if they did float-based, bitmath, or a LUT

iwannabethedev · 2023-05-14T00:29:54Z

I do not feel that it was a good catch, since it seems like an obvious error, that either should have been caught in review or with unit testing. But, it was early in Ruffle's development, and you cannot expect to unit test or carefully code review everything when the code is presumably changing a lot - development-wise, it is less likely to be cost-efficient (though it of course depends on the kind of technology and quality level you are seeking and the goals - at places like NASA, there are of course other priorities).

The code currently fails, and I do not understand why. But, while the new fix visually works with the two SWFs I tested, I have not tested them specifically, so you are right that the new implementation, while not being obviously wrong, may not be accurate to the behavior of the Flash Player, and I agree (if I understand you correctly) that blend_over() really should be fully accurate in regards to what the Flash Player is doing with the Flash Player's version of blend_over(). So my current PR, if the tests are still accurate (and I think they are, because I have now run the AVM2 test script and I do indeed get the values that are in the test folder in output.txt), unless there is an issue or a bug other than in blend_over() elsewhere, then my new changes here are buggy (there might also be the possibility of both my changed implementation of blend_over() and elsewhere being buggy. It would be nice if we could test the corresponding function to blend_over() in the official Flash Player exactly, but I assume that is not possible).

I do not quite understand what you mean by premultiplied alpha, but I think I will have to ponder it a bit before I get it.

iwannabethedev · 2023-05-14T00:33:06Z

Attaching the test result output when running with cargo test copypix in 'tests/tests':

running 2 tests
[WARN  wgpu_hal::vulkan::instance] Unable to find layer: VK_LAYER_KHRONOS_validation
[INFO  naga::back::spv::writer] Skip function Some("find_t")
[INFO  naga::back::spv::writer] Skip function Some("find_t")
[INFO  naga::back::spv::writer] Skip function Some("find_t")
[INFO  naga::back::spv::writer] Skip function Some("find_t")
[INFO  naga::back::spv::writer] Skip function Some("find_t")
[INFO  naga::back::spv::writer] Skip function Some("_naga_oil_mod_MZUWY5DFOI_membermain_vertex")
[INFO  naga::back::spv::writer] Skip function Some("_naga_oil_mod_MZUWY5DFOI_membermain_vertex")
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`: ruffle output != flash player output

Diff < left / right > :
<40142434 40142434 45122134 45122134 45122134 
<40142434 40142434 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 -7f564435 
 transparentSource: -ff1f1f2
 Non-transparent testing
 Original pixel: -1000000
<transparent source mergeAlpha=false -f3f3f4
<transparent source mergeAlpha=true -f3f3f4
>transparent source mergeAlpha=false -f2f2f3
>transparent source mergeAlpha=true -f2f2f3
 nontransparent source mergeAlpha=false -fdfbfa
 nontransparent source mergeAlpha=true -fdfbfa
 
 Transparent testing
 Original pixel: -1
 nontransparent source mergeAlpha=false -fdfbfa
 nontransparent source mergeAlpha=true -fdfbfa
 

', tests/tests/util/test.rs:134:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test avm1/bitmap_data_copypixels ... FAILED
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`: ruffle output != flash player output

Diff < left / right > :
<40142434 40142434 45122134 45122134 45122134 
<40142434 40142434 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 80a9bbcb 
 transparentSource: f00e0e0e
 Non-transparent testing
 Original pixel: ff000000
<transparent source mergeAlpha=false ff0c0c0c
<transparent source mergeAlpha=true ff0c0c0c
>transparent source mergeAlpha=false ff0d0d0d
>transparent source mergeAlpha=true ff0d0d0d
 nontransparent source mergeAlpha=false ff020406
 nontransparent source mergeAlpha=true ff020406
 
 Transparent testing
 Original pixel: ffffffff
 nontransparent source mergeAlpha=false ff020406
 nontransparent source mergeAlpha=true ff020406
 Copy with alphaPoint=undefined:
<fe7f0000 fe7f0000 0 0 0 
<fe7f0000 fe7f0000 0 0 0 
>ff800000 ff800000 0 0 0 
>ff800000 ff800000 0 0 0 
 0 0 0 0 0 
 0 0 0 0 0 
 0 0 0 0 0 
 

', tests/tests/util/test.rs:134:13
test avm2/bitmapdata_copypixels  ... FAILED

failures:

---- avm1/bitmap_data_copypixels ----
test panicked: assertion failed: `(left == right)`: ruffle output != flash player output

Diff < left / right > :
<40142434 40142434 45122134 45122134 45122134 
<40142434 40142434 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 -7f564435 
 transparentSource: -ff1f1f2
 Non-transparent testing
 Original pixel: -1000000
<transparent source mergeAlpha=false -f3f3f4
<transparent source mergeAlpha=true -f3f3f4
>transparent source mergeAlpha=false -f2f2f3
>transparent source mergeAlpha=true -f2f2f3
 nontransparent source mergeAlpha=false -fdfbfa
 nontransparent source mergeAlpha=true -fdfbfa
 
 Transparent testing
 Original pixel: -1
 nontransparent source mergeAlpha=false -fdfbfa
 nontransparent source mergeAlpha=true -fdfbfa
 



---- avm2/bitmapdata_copypixels ----
test panicked: assertion failed: `(left == right)`: ruffle output != flash player output

Diff < left / right > :
<40142434 40142434 45122134 45122134 45122134 
<40142434 40142434 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
>5d243147 5d243147 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 45122134 
 45122134 45122134 45122134 45122134 80a9bbcb 
 transparentSource: f00e0e0e
 Non-transparent testing
 Original pixel: ff000000
<transparent source mergeAlpha=false ff0c0c0c
<transparent source mergeAlpha=true ff0c0c0c
>transparent source mergeAlpha=false ff0d0d0d
>transparent source mergeAlpha=true ff0d0d0d
 nontransparent source mergeAlpha=false ff020406
 nontransparent source mergeAlpha=true ff020406
 
 Transparent testing
 Original pixel: ffffffff
 nontransparent source mergeAlpha=false ff020406
 nontransparent source mergeAlpha=true ff020406
 Copy with alphaPoint=undefined:
<fe7f0000 fe7f0000 0 0 0 
<fe7f0000 fe7f0000 0 0 0 
>ff800000 ff800000 0 0 0 
>ff800000 ff800000 0 0 0 
 0 0 0 0 0 
 0 0 0 0 0 
 0 0 0 0 0 
 




failures:
    avm1/bitmap_data_copypixels
    avm2/bitmapdata_copypixels

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 4 filtered out; finished in 1.17s

error: test failed, to rerun pass `--test tests`

iwannabethedev · 2023-05-14T00:42:08Z

We do all blending in premultiplied alpha,

I do not understand this part. It would explain some cases where to_premultiplied_alpha() is called before-hand (though I feel like renaming and also documenting that blend_over() expects premultiplied alpha values would be helpful, like renaming it blend_over_premultiplied_alpha()). But, while I have only skimmed the code, there appears to be other places where blend_over() is called without to_premultiplied_alpha() being called before-hand. But I will have to take a closer look at the code to be certain I understand what is going on.

iwannabethedev · 2023-05-14T00:57:53Z

Like, in this code piece, it is clear that to_premultiplied_alpha() is called before blend_over().

ruffle/core/src/bitmap/operations.rs

Lines 1329 to 1333 in 476ff4a

    
           color = Color::from(transform * swf::Color::from(color.to_un_multiplied_alpha())) 
        
               .to_premultiplied_alpha(true); 
        
           color = write 
        
               .get_pixel32_raw(dest_region.x_min + x, dest_region.y_min + y) 
        
               .blend_over(&color);

But in this code piece below, I am not certain whether to_premultiplied_alpha() is ever called before blend_over().

ruffle/core/src/bitmap/operations.rs

Lines 1342 to 1355 in 476ff4a

    
           } else { 
        
               let dest = dest.sync(); 
        
               let mut dest_write = dest.write(context.gc_context); 
        
               let source_read = source.read_area(source_region); 
        
               let opaque = !dest_write.transparency(); 
        
               for y in 0..dest_region.height() { 
        
                   for x in 0..dest_region.width() { 
        
                       let mut color = 
        
                           source_read.get_pixel32_raw(source_region.x_min + x, source_region.y_min + y); 
        
                       color = Color::from(transform * swf::Color::from(color)); 
        
                       color = dest_write 
        
                           .get_pixel32_raw(dest_region.x_min + x, dest_region.y_min + y) 
        
                           .blend_over(&color);

As well as here:

ruffle/core/src/bitmap/operations.rs

Lines 1235 to 1245 in 476ff4a

    
           let dest = dest.sync(); 
        
           let mut write = dest.write(context); 
        
           for y in 0..dest_region.height() { 
        
               for x in 0..dest_region.width() { 
        
                   let mut color = 
        
                       write.get_pixel32_raw(source_region.x_min + x, source_region.y_min + y); 
        
                   if blend { 
        
                       color = write 
        
                           .get_pixel32_raw(dest_region.x_min + x, dest_region.y_min + y) 
        
                           .blend_over(&color);

ruffle/core/src/bitmap/operations.rs

Lines 1286 to 1298 in 476ff4a

    
           // Copying (not blending) a transparent texture to an opaque texture, 
        
           // or blending anything to anything 
        
           let opaque = !dest_write.transparency(); 
        
           for y in 0..dest_region.height() { 
        
               for x in 0..dest_region.width() { 
        
                   let mut color = source_read 
        
                       .get_pixel32_raw(source_region.x_min + x, source_region.y_min + y); 
        
                   if blend { 
        
                       color = dest_write 
        
                           .get_pixel32_raw(dest_region.x_min + x, dest_region.y_min + y) 
        
                           .blend_over(&color);

But I am not certain about those places, I have not looked into them carefully.

But; does this mean that the real fix is not this PR, but to carefully ensure that wherever blend_over() is called, to_premultiplied_alpha() has been applied to the values some time before when relevant? If so, I think it would be a really good idea to add documentation to blend_over(), and probably also rename that method, to clearly signal that pre-multiplication is required, since it is wrong for the range of the values of its type as it before this PR is implemented.

iwannabethedev · 2023-05-14T01:17:04Z

@Dinnerbone I think that I will make a new PR, and in that PR, I will not touch the implementation of blend_over(), since if it expects pre-multiplied values, then my changes here are wrong and buggy. Instead, I will see if I can introduce to_premultiplied_alpha() in appropriate places where they appear to be missing, and then run the different SWFs that there have been bugs with - and, to help avoid issues in the future, then document and possibly rename blend_over(), since it feels too error-prone when (as far as I can see) multiple people misunderstand that it expects premultiplied values.

In either case, thank you very much for the help and guidance.

iwannabethedev · 2023-05-14T03:29:38Z

Superseded by #11044 .

core: Fix wrong blend_over() - but with failing tests.

69ba334

iwannabethedev marked this pull request as ready for review May 13, 2023 21:03

iwannabethedev mentioned this pull request May 14, 2023

core: Fix wrong color form used. #11044

Merged

iwannabethedev closed this May 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Fix wrong `blend_over()` - but with failing tests. #11038

core: Fix wrong `blend_over()` - but with failing tests. #11038

iwannabethedev commented May 13, 2023 •

edited

Loading

iwannabethedev commented May 13, 2023

Dinnerbone commented May 13, 2023

iwannabethedev commented May 13, 2023 •

edited

Loading

Dinnerbone commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023 •

edited

Loading

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

core: Fix wrong blend_over() - but with failing tests. #11038

core: Fix wrong blend_over() - but with failing tests. #11038

Conversation

iwannabethedev commented May 13, 2023 • edited Loading

iwannabethedev commented May 13, 2023

Dinnerbone commented May 13, 2023

iwannabethedev commented May 13, 2023 • edited Loading

Dinnerbone commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023 • edited Loading

iwannabethedev commented May 14, 2023

iwannabethedev commented May 14, 2023

core: Fix wrong `blend_over()` - but with failing tests. #11038

core: Fix wrong `blend_over()` - but with failing tests. #11038

iwannabethedev commented May 13, 2023 •

edited

Loading

iwannabethedev commented May 13, 2023 •

edited

Loading

iwannabethedev commented May 14, 2023 •

edited

Loading