148634 - Combine imgContainerGIF::ZeroMaskArea & OneMaskArea

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[ArronM (:paper)]

With the fix of Bug 85595, two functions were added, ZeroMaskArea and
OneMaskArea.  These functions can be optimized and combined into one.  The new
function will be called SetMaskVisibility.  In addition, ZeroMask will be
renamed to SetMaskVisibility as well to keep everything consistant.

This will probably happen after the GIF stuff in imgContainer gets moved to
imgContainerGIF (Bug 77497)

Comment 1

[ArronM (:paper)]

Attachment: SetMaskVisibility

-Remove ZeroMask, ZeroMaskArea, OneMaskArea
-Add SetVisibility counterparts
-add some doxygen comments (more to come in other bug patches)

Comment 2

[ArronM (:paper)]

Attachment: SetMaskInvisibility

Removed debug code and cut long lines into two.

Comment 3

[tor]

Comment on attachment 102192 [details] [diff] [review]
SetMaskInvisibility

The memset optimization is a bit ugly - how often do we actually 
get a count of one?

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

please remove the @brief, as discussed on irc

nice, didn't know \overload. thanks for making me look it up :)
however: from http://www.stack.nl/~dimitri/doxygen/commands.html#cmdoverload :
Note 2:  The \overload command does not work inside a one-line comment.
does that apply here?


will look at code issues later.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

about the patch itself:
  PRUint32 abprComposite;
  aFrame->GetAlphaBytesPerRow(&abprComposite);
as you renamed the argument, I think just "abpr" is a better name for this function.
same for compositingAlphaData and {width,height}Composite
PR_MIN(aWidth,(widthComposite-aX));

please insert a space after the , and around the -
same on the next line
and here around the *:         offset = aY*abprComposite;

        // Should never get run, since gifs are all 1 bit alpha
so why implement it?

especially now that this container will only be used for GIFs.
I would put an NS_NOTREACHED("gifs must be 1 bit alpha"); in the default: block
and remove the RGB_A8 and BGR_A8 block.

um, I notice this comment in your patch (though you didn't touch it)
// Yet another thing that should be in a GIF specific container.
seems out of data, given that this is imgContainerGIF.cpp :)

also I agree with tor

not quite done with the review, will continue later

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

        PRUint8 maskStart; 
this variable will only be initialized if width > numReplacingStart and
maskShiftStartBy != 0. I think you should initialize it here.

similar for maskStart


also, both for maskStart and for maskEnd, I would put the ~maskStart/~maskEnd
into the if, where it will be used, not in the assignment. i.e., change
 maskStart = (aVisible) ? (0xFF >> maskShiftStartBy) : ~(0xFF >> maskShiftStartBy);
to
 maskStart = (0xFF >> maskShiftStartBy);
(same for the other assignments to maskStart)
and later change:
 *localAlpha++ &= maskStart;
to
 *localAlpha++ &= ~maskStart;
(same for maskEnd)


that makes it imho more clear what happens there.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

note, the ResetAnimation patch has bitrotted this one - that function needs a
change as well (trivial to do)

Comment 8

[ArronM (:paper)]

Attachment: SetMaskVisibility

tor:
my pretty 1 byte setting optimization is gone :P

biesi:
re: overload not working on single line.
That's very odd, because it's working for me.  I think they lie. :P  Either
that or it works in this case because it's right above the function. The
example they gave their code documentation was seperate from the class code.

re: abrComposite
oh, blast me and my changing of the parameter from aCompositingFrame to aFrame
without updating the other variable names. ;)  Everything with Composit* has
been removed now.

re: initializing maskStart
maskStart is only used if maskShiftStartBy != 0, and maskStart is initialized
for all those cases.
not sure what you are refering to when you say "similar for maskStart"

re: // Yet another thing..
Yes, those will be removed in the other imgContainerGIF patches I have

Other Changes:
- Moved abpr into case block

- Set alphaLine in WIN&OS2/Other #define so that we always increase alphaLine,
thus not needing more #define switches in the loops (making it easier to read)

- Moved maskShiftEndBy declaration into the code block it's used in

- Split the for loop into two, one for aVisible and one for !aVisible.	Before
aVisible checked every loop incrementation.

- ((Removed (excess ((bracketting)))!))

- unbitrotted

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

> similar for maskStart

er, right, I meant something else, presumably maskEnd

ok, if the @overload works for you, that's fine.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 102714 [details] [diff] [review]
SetMaskVisibility

r=biesi

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 102714 [details] [diff] [review]
SetMaskVisibility

er, forgot to mention, please remove the unused offset variable as mentioned on
irc

Comment 12

[tor]

Comment on attachment 102714 [details] [diff] [review]
SetMaskVisibility

Remove offset per biesi's comment.

Add an initializer to maskStart to quiet the compiler.

sr=tor

Comment 13

[ArronM (:paper)]

Attachment: SetMaskVisibility

removed offset and initialized maskStart (although my compiler doesn't give a
warning)

Comment 14

[ArronM (:paper)]

Comment on attachment 102750 [details] [diff] [review]
SetMaskVisibility

Transferring r=biesi, sr=tor from previous patch

Comment 15

[tor]

Checked in.

Comment 16

[tor]

Closing.