Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GRAPHICS: Optimized managed surface blit #3951

Merged
merged 2 commits into from Jun 13, 2022

Conversation

elasota
Copy link
Contributor

@elasota elasota commented Jun 3, 2022

Improves the performance of managed surface blits by avoiding expensive operations in common cases:

  • Copies the source/destination formats to local variables so the compiler can determine that they're constant in the loop (i.e. that writing to a pixel won't change values in the format), allowing more optimizations.
  • Precomputes an alpha bitmask for the source format to quickly determine if the pixel is opaque or transparent (note that the condition is done in a way that 0 alpha mask is always opaque).
  • Avoids decomposing and recomposing the color if the source and destination are the same format and the pixel is opaque. In that case, just copies the pixel value.
  • Uses cheaper alpha blend math if the destination pixel is opaque.

Copy link
Member

@OMGPizzaGuy OMGPizzaGuy left a comment

I'm taking an interest as I touched some blending in graphics/fonts/ttf.cpp that probably could use similar optimizations.
Do we have a way to measure performance improvements for best case & worst cases?

if (srcFormat.bytesPerPixel == 1) {
assert(srcPalette != nullptr); // Catch the cases when palette is missing
// Get the palette color
col = srcPalette[*srcVal];
Copy link
Member

@OMGPizzaGuy OMGPizzaGuy Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assert and assignment to col already handled earlier?

Copy link
Member

@sev- sev- Jun 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems like line 374.

if (srcFormat.bytesPerPixel == 2)
col = *reinterpret_cast<const uint16 *>(srcVal);
else
col = *reinterpret_cast<const uint32 *>(srcVal);
Copy link
Member

@OMGPizzaGuy OMGPizzaGuy Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this assignment to col already handled earlier?

Copy link
Member

@sev- sev- Jun 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 380-383

@sev-
Copy link
Member

@sev- sev- commented Jun 12, 2022

The proposed changes are good, but there seem to be a duplicated logic

@sev-
Copy link
Member

@sev- sev- commented Jun 13, 2022

Great, thank you!

@sev- sev- merged commit 8c560fe into scummvm:master Jun 13, 2022
8 checks passed
bDest = destVal[2];
// Partially transparent, so calculate new pixel colors
if (destFormat.bytesPerPixel == 2) {
uint32 destColor = *reinterpret_cast<uint16 *>(destVal);
Copy link
Contributor

@antoniou79 antoniou79 Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this just got merged, but this seems like some oversight(?)
Is destColor just unused in this case?

Copy link
Member

@OMGPizzaGuy OMGPizzaGuy Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks like a format.colorToARGB call got cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants