Closed
Bug 367281
Opened 18 years ago
Closed 17 years ago
Remove SetImageData/SetAlphaData from gfxImageFrame
Categories
(Core Graveyard :: GFX, defect)
Core Graveyard
GFX
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.9beta1
People
(Reporter: alfredkayser, Assigned: alfredkayser)
References
Details
(Keywords: perf)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•17 years ago
|
||
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,
Assignee | ||
Comment 2•17 years ago
|
||
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.
Assignee | ||
Comment 4•17 years ago
|
||
Put the correct bug for non-cairo removal for XBM decoding in dep.list.
Assignee | ||
Comment 5•17 years ago
|
||
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.
Status: NEW → ASSIGNED
Summary: With Cairo, gfxImageFrame can be simplified. → Remove SetImageData/SetAlphaData from gfxImageFrame
Assignee | ||
Comment 6•17 years ago
|
||
This cleanup saves 3K, and removes old code that no longer works (SetAlpha and friends don't work in Cairo/Thebes)
Assignee: general → alfredkayser
Attachment #277545 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #277545 -
Flags: review?(pavlov)
Attachment #277545 -
Flags: review+
Attachment #277545 -
Flags: approval1.9+
Assignee | ||
Updated•17 years ago
|
Attachment #277545 -
Flags: superreview?(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 on attachment 277545 [details] [diff] [review] Remove unused members from gfxImageFrame Previous comment should be handled in a separate bug.
Attachment #277545 -
Flags: superreview?(tor) → superreview+
Assignee | ||
Comment 9•17 years ago
|
||
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.
Keywords: checkin-needed
Comment 10•17 years ago
|
||
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.
Keywords: checkin-needed
Assignee | ||
Comment 11•17 years ago
|
||
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).
Assignee | ||
Comment 13•17 years ago
|
||
Oopsie, wrong patch attach. This one is the right one!
Attachment #282664 -
Attachment is obsolete: true
Assignee | ||
Comment 15•17 years ago
|
||
Now that all 'Depends on' bugs are fixed, this one can now be checked in. Thanks in advance, Alfred
Blocks: 393001
Keywords: checkin-needed
Comment 16•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Assignee | ||
Comment 17•17 years ago
|
||
Thanks! libgkgfxthebes.so Total: -1480 (+0/-1480) Code: -1357 (+0/+0) Data: -123 (+0/-1480) 1.5K codesize savings!
Assignee | ||
Updated•17 years ago
|
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Flags: in-testsuite-
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•