367281 - Remove SetImageData/SetAlphaData from gfxImageFrame

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect)

Description

[Alfred Kayser]

With Cairo, gfxImageFrame can be simplified.

Parts:
* Remove all alpha stuff as they don't have meaning in Cairo
* Remove all Lock/Unlock pixels as they don't have meaning in Cairo
* Either remove SetImageData or just make it memcpy in the pointer of GetImageData
* Add a 'ImageUpdated' helper to access the ImageUpdated of mImage,
    so that image decoders can just do mFrame->ImageUpdated(rect);
    after having set pixels via the pointer from GetImageData
* Set/GetBackgroundColor can be removed, as it is nowhere used
* Replace 'gfxformat' with just alphadepth (0,1,8)

This all to happen when codebase is fully switched to Cairo

Comment 1

[Alfred Kayser]

Updated and more thorough analysis of what can be cleaned in gfxImageFrame

    X,Y: (readonly)
        Nowhere used, can be removed (but looks still usefull)
    
    SetImageData: 'depreciated'
Bug 334144: /modules/libpr0n/decoders/bmp/nsICODecoder.cpp, line 83 -- nsresult nsICODecoder::SetImageData()
Bug 334144: /modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp, line 141 -- nsresult rv = mFrame->SetImageData(mDecoded, mBpr, line * mBpr);
bug 366465: /modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp, line 466 -- mImageFrame->SetImageData(mRGBLine, bpr, (aRowNumber+i)*bpr);
bug 361299: /modules/libpr0n/decoders/icon/nsIconDecoder.cpp, line 148 -- mFrame->SetImageData(data, bpr, rownum * bpr);
bug 376471: /modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp, line 280 -- mFrame->SetImageData(mAlphaRow, abpr, mCurRow * abpr);
bug 385891: /layout/generic/nsFrame.cpp, line 416 -- image->SetImageData(row_data, bpr, i*bpr);

    SetAlphaData: 'no use in Cairo'
bug 385891:  /layout/generic/nsFrame.cpp, line 415 -- image->SetAlphaData(alpha, abpr, i*abpr);

    GetAlphaData/GetAlphaDataLength: 'no use in Cairo'
cvs remove: /modules/libpr0n/decoders/gif/imgContainerGIF.cpp, line 703 -- res = aCompositingFrame->GetAlphaData(&compositingAlphaData,
bug 385883: /modules/libpr0n/src/imgRequest.cpp, line 544 -- frame->GetAlphaDataLength(&alphaSize);
bug 385888: /widget/src/windows/nsWindow.cpp, line 2948 -- rv = frame->GetAlphaData(&adata, &dataLen);
bug 385888: /widget/src/os2/nsWindow.cpp, line 1931 -- rv = frame->GetAlphaData(&adata, &dataLen);

    LockAlpha/UnlockAlphaData: 'no use in Cairo'
bug 385886: /extensions/canvas3d/src/nsCanvasRenderingContextGL.cpp, line 857 -- rv |= frame->LockAlphaData();
cvs remove: /modules/libpr0n/decoders/gif/imgContainerGIF.cpp, line 702 -- aCompositingFrame->LockAlphaData();
bug 385888: /widget/src/windows/nsWindow.cpp, line 2947 -- frame->LockAlphaData();
bug 385888: /widget/src/os2/nsWindow.cpp, line 1930 -- frame->LockAlphaData();

    GetBackgroundColor/SetBackgroundColor:
        Not used!

    GetAlphaBytesPerRow:
Bug 334144: /modules/libpr0n/decoders/bmp/nsBMPDecoder.cpp, line 156 -- nsresult rv = mFrame->GetAlphaBytesPerRow(&abpr);
cvs remove: /modules/libpr0n/decoders/gif/imgContainerGIF.cpp, line 516 -- nextFrameHasAlpha = NS_SUCCEEDED(aNextFrame->GetAlphaBytesPerRow(&aBPR));
bug 361299: /modules/libpr0n/decoders/icon/nsIconDecoder.cpp, line 139 -- mFrame->GetAlphaBytesPerRow(&abpr);
bug 361299: /modules/libpr0n/decoders/icon/beos/nsIconChannel.cpp, line 239 -- PRUint32 alphaBytesPerRow = (iconSize / 8);
bug 376471: /modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp, line 214 -- mFrame->GetAlphaBytesPerRow(&abpr);

Only used to test for transparency, should be replaced by GetFormat
/modules/libpr0n/src/imgContainer.cpp, line 578 -- nextFrameHasAlpha = NS_SUCCEEDED(aNextFrame->GetAlphaBytesPerRow(&aBPR));

bug 385891: /layout/generic/nsFrame.cpp, line 380 -- image->GetAlphaBytesPerRow(&abpr);
bug 385888: /widget/src/windows/nsWindow.cpp, line 2594 -- PRUint32 aAlphaBytesPerRow,
bug 385888: /widget/src/windows/nsWindow.h, line 510 -- PRUint8* aAlphaData, PRUint32 aAlphaBytesPerRow,

Comment 2

[Alfred Kayser]

Also  DrawTo can be moved to from gfxImageFrame.idl to imgContainer.h (just like ClearFrame and CopyFrameImage are in imgContainer.cpp). This removes one function from the .IDL, as DrawTo no where used, except in imgContainer itself.

Comment 3

[Alfred Kayser]

See bug 216682 for DrawTo/DrawToImage removal.

Comment 4

[Alfred Kayser]

Put the correct bug for non-cairo removal for XBM decoding in dep.list.

Comment 5

[Alfred Kayser]

Summary:
    SetImageData: 'depreciated'
    SetAlphaData: 'no use in Cairo'
    GetAlphaData/GetAlphaDataLength: 'no use in Cairo'
    LockAlpha/UnlockAlphaData: 'no use in Cairo'
    GetBackgroundColor/SetBackgroundColor: Not used!
    GetAlphaBytesPerRow: No use in Cairo
Essential all 'alpha' things can be removed, and the Set*Data is depreciated, and Background is not used.

Comment 6

[Alfred Kayser]

Attachment: Remove unused members from gfxImageFrame

This cleanup saves 3K, and removes old code that no longer works (SetAlpha and friends don't work in Cairo/Thebes)

Comment 7

[tor]

Comment on attachment 277545 [details] [diff] [review]
Remove unused members from gfxImageFrame

>--- gfx/src/shared/gfxImageFrame.h	23 Feb 2006 09:36:26 -0000	1.11
>+++ gfx/src/shared/gfxImageFrame.h	21 Aug 2007 15:52:34 -0000
>@@ -66,26 +66,18 @@ public:
> 
>   gfxImageFrame();
>   virtual ~gfxImageFrame();
> 
> protected:
>   nsIntSize mSize;
> 
> private:
>-  nsresult  SetData(const PRUint8 *aData, PRUint32 aLength, 
>-                    PRInt32 aOffset, PRBool aSetAlpha);
>-
>   nsCOMPtr<nsIImage> mImage;
> 
>   PRPackedBool mInitialized;
>   PRPackedBool mMutable;
>-  PRPackedBool mHasBackgroundColor;
>-  PRPackedBool mTopToBottom;
>   gfx_format   mFormat;
> 
>-  PRInt32 mTimeout; // -1 means display forever
>-  nsIntPoint mOffset;
>-
>-  gfx_color mBackgroundColor;
>-
>-  PRInt32   mDisposalMethod;
>+  PRInt32      mTimeout; // -1 means display forever
>+  nsIntPoint   mOffset;
>+  PRInt32      mDisposalMethod;
> };

Since gfx_format is an IDL "long" (why? seems like a shorter one would be fine for the eight possible values), the ordering of this struct should probably be this for minimal size:

nsIntPoint   mOffset;
nsCOMPtr<nsIImage> mImage;
gfx_format   mFormat;
PRInt32      mTimeout; // -1 means display forever
PRInt32      mDisposalMethod;
PRPackedBool mInitialized;
PRPackedBool mMutable;

Comment 8

[tor]

Comment on attachment 277545 [details] [diff] [review]
Remove unused members from gfxImageFrame

Previous comment should be handled in a separate bug.

Comment 9

[Alfred Kayser]

Actually re-ordering won't reduce size, as all fields except the PackedBool's are (multiple of) DWORD's. Only by also changing mFormat to PRUint8 and mDisposalMethod to PRUint8 we could save about 8 bytes. 

Comment 10

[Nickolay_Ponomarev]

Please specify dependencies when using checkin-needed. This patch can't be checked in on its own - it at least depends on the patch in bug 391643. There's also an alphaBytesPerRow hit in nsXBMDecoder.cpp, which I didn't find a bug for, and I don't know what else.

Comment 11

[Alfred Kayser]

The open bugs in the dep-list all need to be closed to checkin this patch.
They are all waiting on Approval1.9 or (super)reviews.
(Note bug 385886 is about Canvas3d which is not part of build, so not neccesary a blocking dependency).

Comment 12

[Alfred Kayser]

Attachment: V2: Update to current trunk

Comment 13

[Alfred Kayser]

Attachment: V2: Right patch

Oopsie, wrong patch attach. This one is the right one!

Comment 14

[Alfred Kayser]

This patch can be committed as soon as bug 376471 is committed.

Comment 15

[Alfred Kayser]

Now that all 'Depends on' bugs are fixed, this one can now be checked in.
Thanks in advance, Alfred

Comment 16

[Reed Loden [:reed]]

Checking in gfx/idl/gfxIImageFrame.idl;
/cvsroot/mozilla/gfx/idl/gfxIImageFrame.idl,v  <--  gfxIImageFrame.idl
new revision: 1.17; previous revision: 1.16
done
Checking in gfx/src/shared/gfxImageFrame.cpp;
/cvsroot/mozilla/gfx/src/shared/gfxImageFrame.cpp,v  <--  gfxImageFrame.cpp
new revision: 1.41; previous revision: 1.40
done
Checking in gfx/src/shared/gfxImageFrame.h;
/cvsroot/mozilla/gfx/src/shared/gfxImageFrame.h,v  <--  gfxImageFrame.h
new revision: 1.12; previous revision: 1.11
done

Comment 17

[Alfred Kayser]

Thanks!

  libgkgfxthebes.so
  	Total:	      -1480 (+0/-1480)
  	Code:	      -1357 (+0/+0)
  	Data:	       -123 (+0/-1480)
1.5K codesize savings!