Closed
Bug 513681
Opened 15 years ago
Closed 14 years ago
Eliminate repeated code with image decoder superclass
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta5+ |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(17 files, 6 obsolete files)
There's a bunch of stuff that all of the decoders do, and that's usually where we end up making behavioral changes. If this were abstracted into a superclass, the decoders would be easier to maintain, and it would be easier to check behavioral invariants in regard to initialization, closing, etc.
Assignee | ||
Updated•15 years ago
|
Summary: Eliminated repeated code with image decoder superclass → Eliminate repeated code with image decoder superclass
Assignee | ||
Comment 1•15 years ago
|
||
this should be the "right fix" to bug 516265.
Assignee | ||
Comment 2•14 years ago
|
||
One thing that would be nice to do with this would be to clear things up in the area of decode completion (does the decoder call DecodingComplete() when it gets enough data? or just when it's closed?). This would be helpful for bug 511899.
Assignee | ||
Comment 3•14 years ago
|
||
nominating - blocks a blocker.
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → beta5+
Assignee | ||
Comment 4•14 years ago
|
||
This small patch just fixes a few harmless Imagelib warnings. I included it at the base of this stack so that I could be sure I wasn't introducing any warnings with my patches.
Assignee | ||
Comment 5•14 years ago
|
||
We're about to consolidate and deCOM the image decoders, so it doesn't really make sense to support disabling certain decoders at build time. The only decoders with any overhead are png an jpeg (because of libpng and libjpeg), and those formats are pretty integral to the platform. bz was ok with this.
Assignee | ||
Comment 6•14 years ago
|
||
We're deCOM-ing image decoders, so we can't instantiate them with do_CreateInstance anymore. Use a simple switch statement instead.
Assignee | ||
Comment 7•14 years ago
|
||
This is the main patch to deCOM the image decoders. We've already decided on this in various discussions, but I'll record the reasoning here for posterity. Historically, each image decoder was a separate XPCOM component that implemented the imgIDecoder interface. This was done so that new decoders could be added as binary components, which would presumably be useful to embedders and for extensions. Cursory research indicates that this functionality has been used occasionally, but not all that much, and not in any code that's been updated in the last 5 years. Since imgIContainer changed significantly in 1.9.2, it's safe to say that this code has been broken since at least Firefox 3.6. Unfortunately, this modularization also came with a price. There's a ton of duplicated code between the decoders that makes maintaining the underbelly of imagelib a nightmare. Each decoder behaves similarly but ever-so-slightly-differently, which makes features such as consistent error recovery (bug 514033) nearly impossible to implement. Moreover, we've had a number of security bugs related to different decoder behavior in edge-cases. So we really want a decoder superclass, and want to assume that we're dealing with precisely that superclass from the rest of Imagelib. We'd also rather that the API here not be COM-y. In theory, we still could offer an XPCOM interface that the superclass implements, and assume that any hypothetical extensions act exactly like the superclass, even though they can't inherit it cross-module. However, this implies various stability guarantees that we don't really want to make (for example, that these assumptions won't change between minor versions). Imagelib is perennially understaffed, and we just don't have the resources to support this sort of extensibility. It's nice in theory, but when pitted against the prospect having a more stable, secure, maintainable image-rendering library, it doesn't hold much weight. Looking at things another way, the ability to have external image decoders was lost on trunk a while ago when we superclassed imgContainer to support <image src="foo.svg"> (bug 584841). Nobody batted an eyelash when this happened (sr+), because external image decoders aren't something we think about, nor do we have any sort of test coverage for them. This patch just does some welcome cleanup made possible by the implicit decision to un-support this functionality.
Assignee | ||
Comment 8•14 years ago
|
||
This patch moves the decoders out of their subdirectories. They're annoying to deal with in subdirectories, and gecko is moving towards flat-ness.
Assignee | ||
Comment 9•14 years ago
|
||
This patch introduces the decoder superclass that all the decoders will eventually inherit from.
Assignee | ||
Comment 10•14 years ago
|
||
Most of the decoders refer to their imgIContainer as mImage, but the GIF decoder refers to it as mImageContainer (speaking of annoying inconsistencies and repeated code). We'll soon move mImage into the superclass, which will break the GIF decoder unless it also operates on mImage.
Assignee | ||
Comment 11•14 years ago
|
||
imgIContainer::Flush() is a no-op everywhere but the JPEG decoder (where its use also happens to be somewhat dubious). Flush() is only ever called by RasterImage right before it calls Close(), so we can move the logic in nsJPEGDecoder::Flush() into the beginning of nsJPEGDecoder::Close() and achieve the same effect. We want to do this so that we can eliminate Flush().
Assignee | ||
Comment 12•14 years ago
|
||
nsPNGDecoder::mImage is currently public to allow static libpng callback functions to access it. When we move mImage into the superclass, it will be protected, so the PNG decoder will break as-is. Make the callbacks static methods of nsPNGDecoder so that this type of access is allowed.
Assignee | ||
Comment 13•14 years ago
|
||
This patch makes each of the decoders inherit from the superclass, and moves various bits of common functionality into the superclass.
Assignee | ||
Comment 14•14 years ago
|
||
Now that all the decoders implement mozilla::imagelib::Decoder, RasterImage can know that it's holding onto something more concrete than an imgIDecoder.
Assignee | ||
Comment 15•14 years ago
|
||
We're going to eliminate the notion of decoder initialization flags soon. Right now, we store the decoder initialization flags in RasterImage to keep track of whether we're doing a headeronly decode. We'll soon just query the decoder, but this only works when the decoder is non-null. This patch fixes one place in RasterImage where that assumption doesn't quite hold, and then asserts that it does.
Assignee | ||
Comment 16•14 years ago
|
||
This patch eschews the use of flags for indicating the decode type and instead uses a simple getter/setter (this is the new hotness in places like layout). Also, "headeronly" was a crappy name, so this patch changes the name to "size decode".
Assignee | ||
Comment 17•14 years ago
|
||
One common thing that decoders do is set the size on the RasterImage, and then send the OnStartContainer notification. This patch coalesces that functionality into the superclass.
Assignee | ||
Comment 18•14 years ago
|
||
Similar to the previous patch, this patch moves the logic of beginning and ending frames into the superclass.
Assignee | ||
Comment 19•14 years ago
|
||
A longstanding source of pain has been that imgIDecoder::Shutdown() is dual-natured. Most implementations include some sort of notification code that should not be called in error or size-only cases. However, some implementations also include cleanup code, such that _not_ calling Shutdown() will leak. This patch separates those two tasks, so that the cleanup code goes in the destructor and the finalization code goes into Decoder::Finish().
Assignee | ||
Comment 20•14 years ago
|
||
As the crowning achievement of this stack, this final patch eliminates imgIDecoder. Woohoo!
Assignee | ||
Updated•14 years ago
|
Attachment #467658 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467659 -
Flags: review?(me)
Attachment #467659 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467660 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467661 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467662 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467663 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467664 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467665 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467666 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467667 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467668 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467669 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467670 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467671 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467672 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467673 -
Flags: review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #467674 -
Flags: review?(joe)
Updated•14 years ago
|
Attachment #467658 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467659 -
Flags: review?(joe) → review+
Comment 21•14 years ago
|
||
Comment on attachment 467660 [details] [diff] [review] part 3 - v1 - instantiate decoders without using COM goop >+ // Instantiate the appropriate decoder >+ switch (type) { >+ case eDecoderType_png: >+ mDecoder = new nsPNGDecoder(); >+ break; >+ case eDecoderType_gif: >+ mDecoder = new nsGIFDecoder2(); >+ break; >+ case eDecoderType_jpeg: >+ mDecoder = new nsJPEGDecoder(); >+ break; >+ case eDecoderType_bmp: >+ mDecoder = new nsBMPDecoder(); >+ break; >+ case eDecoderType_ico: >+ mDecoder = new nsICODecoder(); >+ break; >+ case eDecoderType_icon: >+ mDecoder = new nsIconDecoder(); >+ break; >+ default: >+ NS_ABORT_IF_FALSE(0, "Shouldn't get here!"); I so wish we could write NS_ABORT("Shouldn't get here!") >--- a/modules/libpr0n/src/imgLoader.cpp >+++ b/modules/libpr0n/src/imgLoader.cpp >@@ -1853,14 +1853,11 @@ NS_IMETHODIMP imgLoader::LoadImageWithChannel(nsIChannel *channel, imgIDecoderOb > NS_IMETHODIMP imgLoader::SupportImageWithMimeType(const char* aMimeType, PRBool *_retval) > { > *_retval = PR_FALSE; >- nsCOMPtr<nsIComponentRegistrar> reg; >- nsresult rv = NS_GetComponentRegistrar(getter_AddRefs(reg)); >- if (NS_FAILED(rv)) >- return rv; > nsCAutoString mimeType(aMimeType); > ToLowerCase(mimeType); >- nsCAutoString decoderId(NS_LITERAL_CSTRING("@mozilla.org/image/decoder;3?type=") + mimeType); >- return reg->IsContractIDRegistered(decoderId.get(), _retval); >+ *_retval = (RasterImage::GetDecoderType(mimeType.get()) == RasterImage::eDecoderType_unknown) >+ ? PR_FALSE : PR_TRUE; >+ return NS_OK; This code in imgLoader makes me think that the DecoderType stuff should be in Image/imgIContainer, since SupportImageWithMimeType will want to return true for SVG images too.
Attachment #467660 -
Flags: review?(joe) → review+
Comment 22•14 years ago
|
||
Comment on attachment 467661 [details] [diff] [review] part 4 - v1 - Get rid of CIDs and friends for decoders. 0 insertions(+), 164 deletions(-) = r+
Attachment #467661 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467662 -
Flags: review?(joe) → review+
Comment 23•14 years ago
|
||
Comment on attachment 467663 [details] [diff] [review] part 6 - v1 - Introduce Decoder.cpp and Decoder.h >From c4a658cd25f94e824cdf8d2b55eae2f4321e794e Mon Sep 17 00:00:00 2001 >From: Bobby Holley <bobbyholley@gmail.com> >Date: Tue, 10 Aug 2010 20:11:12 -0400 >Subject: [PATCH] part 6 - v1 - Introduce Decoder.cpp and Decoder.h > >This patch introduces the decoder superclass that all the decoders will eventually inherit from. >--- > modules/libpr0n/src/Decoder.cpp | 135 ++++++++++++++++++++++++++++++++++++++ > modules/libpr0n/src/Decoder.h | 138 +++++++++++++++++++++++++++++++++++++++ > modules/libpr0n/src/Makefile.in | 1 + > 3 files changed, 274 insertions(+), 0 deletions(-) > create mode 100644 modules/libpr0n/src/Decoder.cpp > create mode 100644 modules/libpr0n/src/Decoder.h > >diff --git a/modules/libpr0n/src/Decoder.cpp b/modules/libpr0n/src/Decoder.cpp >new file mode 100644 >index 0000000..94f84a5 >--- /dev/null >+++ b/modules/libpr0n/src/Decoder.cpp >@@ -0,0 +1,135 @@ >+ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is mozilla.org code. >+ * >+ * The Initial Developer of the Original Code is >+ * the Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010. >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+#include "Decoder.h" >+ >+namespace mozilla { >+namespace imagelib { >+ >+// XXX - This goes away when we stop implementing imgIDecoder >+NS_IMPL_ISUPPORTS1(Decoder, imgIDecoder) >+ >+/* >+ * Translation layer from imgIDecoder to the interface we're actually going to use. >+ */ >+NS_IMETHODIMP >+Decoder::Init(imgIContainer *aImage, >+ imgIDecoderObserver *aObserver, >+ PRUint32 aFlags) >+{ >+ NS_ABORT_IF_FALSE(aImage->GetType() == imgIContainer::TYPE_RASTER, >+ "wrong type of imgIContainer for decoding into"); >+ return Init(static_cast<RasterImage*>(aImage), aObserver, aFlags); >+} >+ >+NS_IMETHODIMP >+Decoder::Close(PRUint32 aFlags) >+{ >+ return Shutdown(aFlags); >+} >+ >+NS_IMETHODIMP Decoder::Flush() >+{ >+ return NS_OK; >+} >+ >+Decoder::Decoder() >+ : mInitialized(false) >+{ >+} >+ >+Decoder::~Decoder() >+{ >+ mInitialized = false; >+} >+ >+/* >+ * Common implementation of the decoder interface. >+ */ >+ >+nsresult >+Decoder::Init(RasterImage* aImage, imgIDecoderObserver* aObserver, >+ PRUint32 aFlags) >+{ >+ // We should always have an image >+ NS_ABORT_IF_FALSE(aImage, "Can't initialize decoder without an image!"); >+ >+ // Save our paremeters >+ mImage = aImage; >+ mObserver = aObserver; >+ mFlags = aFlags; >+ >+ // Implementation-specific initialization >+ nsresult rv = InitInternal(); >+ mInitialized = true; >+ return rv; >+} >+ >+// XXX - This should stop being an IMETHODIMP when we stop inheriting imgIDecoder >+NS_IMETHODIMP >+Decoder::Write(const char* aBuffer, PRUint32 aCount) >+{ >+ // Pass the data along to the implementation >+ return WriteInternal(aBuffer, aCount); >+} >+ >+nsresult >+Decoder::Finish() >+{ >+ // Implementation-specific finalization >+ return FinishInternal(); >+} >+ >+nsresult >+Decoder::Shutdown(PRUint32 aFlags) >+{ >+ // Implementation-specific shutdown >+ return ShutdownInternal(aFlags); >+} >+ >+/* >+ * Hook stubs. Override these as necessary in decoder implementations. >+ */ >+ >+nsresult Decoder::InitInternal() {return NS_OK; } >+nsresult Decoder::WriteInternal(const char* aBuffer, PRUint32 aCount) {return NS_OK; } >+nsresult Decoder::FinishInternal() {return NS_OK; } >+nsresult Decoder::ShutdownInternal(PRUint32 aFlags) {return NS_OK; } >+ >+} // namespace imagelib >+} // namespace mozilla >diff --git a/modules/libpr0n/src/Decoder.h b/modules/libpr0n/src/Decoder.h >new file mode 100644 >index 0000000..5ed5358 >--- /dev/null >+++ b/modules/libpr0n/src/Decoder.h >@@ -0,0 +1,138 @@ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is mozilla.org code. >+ * >+ * The Initial Developer of the Original Code is >+ * the Mozilla Foundation. >+ * Portions created by the Initial Developer are Copyright (C) 2010. >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): >+ * Bobby Holley <bobbyholley@gmail.com> >+ * >+ * Alternatively, the contents of this file may be used under the terms of >+ * either the GNU General Public License Version 2 or later (the "GPL"), or >+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"), >+ * in which case the provisions of the GPL or the LGPL are applicable instead >+ * of those above. If you wish to allow use of your version of this file only >+ * under the terms of either the GPL or the LGPL, and not to allow others to >+ * use your version of this file under the terms of the MPL, indicate your >+ * decision by deleting the provisions above and replace them with the notice >+ * and other provisions required by the GPL or the LGPL. If you do not delete >+ * the provisions above, a recipient may use your version of this file under >+ * the terms of any one of the MPL, the GPL or the LGPL. >+ * >+ * ***** END LICENSE BLOCK ***** */ >+ >+#ifndef MOZILLA_IMAGELIB_DECODER_H_ >+#define MOZILLA_IMAGELIB_DECODER_H_ >+ >+#include "RasterImage.h" >+ >+#include "imgIDecoderObserver.h" >+#include "imgIDecoder.h" >+ >+namespace mozilla { >+namespace imagelib { >+ >+// XXX - We temporarily inherit imgIDecoder >+class Decoder : public imgIDecoder >+{ >+public: >+ >+ // XXX - We should get rid of these when westop inheriting imgIDecoder >+ NS_DECL_ISUPPORTS >+ NS_DECL_IMGIDECODER >+ >+ Decoder(); >+ ~Decoder(); >+ >+ /** >+ * XXX - These methods will stop returning nsresults in a later patch. >+ */ >+ >+ /** >+ * Initialize an image decoder. >+ * >+ * @param aContainer The image container to decode to. >+ * @param aObserver The observer for decode notification events. >+ * @param aFlags Flags for the decoder >+ * >+ * Notifications Sent: TODO >+ */ >+ nsresult Init(RasterImage* aImage, imgIDecoderObserver* aObserver, >+ PRUint32 aFlags); >+ >+ /** >+ * Writes data to the decoder. >+ * >+ * @param aBuffer buffer containing the data to be written >+ * @param aCount the number of bytes to write >+ * >+ * Any errors are reported by setting the appropriate state on the decoder. >+ * >+ * Notifications Sent: TODO >+ */ >+ // This is commented out while we inherit imgIDecoder because the signature >+ // is the same as the one in the idl. >+ // nsresult Write(const char* aBuffer, PRUint32 aCount); >+ >+ /** >+ * Informs the decoder that all the data has been written. >+ * >+ * Notifications Sent: TODO >+ */ >+ nsresult Finish(); >+ >+ /** >+ * Shuts down the decoder. >+ * >+ * Notifications Sent: None >+ * >+ * XXX - These flags will go away later in the patch stack >+ */ >+ nsresult Shutdown(PRUint32 aFlags); >+ >+ // We're not COM-y, so we don't get refcounts by default >+ // XXX - This is uncommented in a later patch when we stop inheriting imgIDecoder >+ // NS_INLINE_DECL_REFCOUNTING(Decoder) >+ >+protected: >+ >+ /* >+ * Internal hooks. Decoder implementations may override these and >+ * only these methods. >+ */ >+ virtual nsresult InitInternal(); >+ virtual nsresult WriteInternal(const char* aBuffer, PRUint32 aCount); >+ virtual nsresult FinishInternal(); >+ virtual nsresult ShutdownInternal(PRUint32 aFlags); >+ >+ /* >+ * Member variables. >+ * >+ * XXX - Some of these become private later in the patch stack. >+ */ >+ nsRefPtr<RasterImage> mImage; >+ nsCOMPtr<imgIDecoderObserver> mObserver; >+ PRUint32 mFlags; >+ >+ bool mInitialized; >+}; >+ >+} // namespace imagelib >+} // namespace mozilla >+ >+#endif // MOZILLA_IMAGELIB_DECODER_H_ >diff --git a/modules/libpr0n/src/Makefile.in b/modules/libpr0n/src/Makefile.in >index 369f01b..d16e57e 100644 >--- a/modules/libpr0n/src/Makefile.in >+++ b/modules/libpr0n/src/Makefile.in >@@ -52,6 +52,7 @@ LIBXUL_LIBRARY = 1 > > CPPSRCS = \ > Image.cpp \ >+ Decoder.cpp \ > DiscardTracker.cpp \ > RasterImage.cpp \ > imgFrame.cpp \ >-- >1.7.1.1
Attachment #467663 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467664 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467665 -
Flags: review?(joe) → review+
Comment 24•14 years ago
|
||
Comment on attachment 467666 [details] [diff] [review] part 9 - v1 - Move PNG callbacks into decoder class Please fully qualify the callback names as you use them - eg change from info_callback to nsPNGDecoder::info_callback.
Attachment #467666 -
Flags: review?(joe) → review+
Comment 25•14 years ago
|
||
(In reply to comment #21) > Comment on attachment 467660 [details] [diff] [review] > >+ default: > >+ NS_ABORT_IF_FALSE(0, "Shouldn't get here!"); > > I so wish we could write NS_ABORT("Shouldn't get here!") NS_NOTREACHED?
Updated•14 years ago
|
Attachment #467667 -
Flags: review?(joe) → review+
Comment 26•14 years ago
|
||
Comment on attachment 467668 [details] [diff] [review] part 11 - v1 - make RasterImage carry a pointer to a Decoder Aren't there some includes we could remove in this patch?
Attachment #467668 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467669 -
Flags: review?(joe) → review+
Updated•14 years ago
|
Attachment #467670 -
Flags: review?(joe) → review+
Comment 27•14 years ago
|
||
Comment on attachment 467671 [details] [diff] [review] part 14 - v1 - Coalesce size-setting into superclass Please document what PostSize actually does under the covers (more than just "Progress notifications.") :) I'm a bit concerned that PostSize doesn't check result values; if, for example, OnStartContainer returns an error, we will continue decoding now when, in the past, we would have aborted. I'm not super-concerned about it, though, because not every decoder did that.
Attachment #467671 -
Flags: review?(joe) → review+
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #25) > (In reply to comment #21) > > NS_NOTREACHED? NS_NOTREACHED is, unfortunately, not fatal at the moment.
Comment 29•14 years ago
|
||
Comment on attachment 467672 [details] [diff] [review] part 15 - v1 - Coalesce OnStartFrame/OnStopFrame into superclass Same comments as earlier patch, only on PostFrameStart/PostFrameStop documentation.
Attachment #467672 -
Flags: review?(joe) → review+
Comment 30•14 years ago
|
||
Comment on attachment 467673 [details] [diff] [review] part 16 - v1 - Move end-of-decode logic into Finish() and cleanup into destructor, abolishing Shutdown() This now means we can never reuse decoders. That's fine, but we should make sure it a) isn't done anywhere and b) is documented as such. (I am concerned about multipart/x-mixed-replace wrt part a.)
Attachment #467673 -
Flags: review?(joe) → review+
Comment 31•14 years ago
|
||
Comment on attachment 467674 [details] [diff] [review] part 17 - v1 - remove imgIDecoder Be sure to push this to try, because I'm not sure whether nsRefPtr works properly with only forward-declared classes; I know nsCOMPtr doesn't.
Attachment #467674 -
Flags: review?(joe) → review+
Comment on attachment 467659 [details] [diff] [review] part 2 - v1 - remove support for the MOZ_IMG_DECODERS build option >diff --git a/modules/libpr0n/decoders/Makefile.in b/modules/libpr0n/decoders/Makefile.in >index 55081f9..d43f2b1 100644 >--- a/modules/libpr0n/decoders/Makefile.in >+++ b/modules/libpr0n/decoders/Makefile.in >@@ -42,7 +42,8 @@ VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > >-ifneq (,$(filter icon,$(MOZ_IMG_DECODERS))) >+DECODERS = png gif jpeg bmp >+ > ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT))) > DIRS = icon/gtk icon > endif >@@ -61,9 +62,8 @@ endif > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT)) > DIRS = icon/mac icon > endif >-endif # icon > >-DIRS += $(filter-out icon,$(MOZ_IMG_DECODERS)) >+DIRS += $(DECODERS) > > include $(topsrcdir)/config/rules.mk Nit: remove DECODERS entirely here and list the directories explicitly on DIRS. r=me.
Attachment #467659 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 33•14 years ago
|
||
(In reply to comment #32) > Nit: remove DECODERS entirely here and list the directories explicitly on DIRS. > > r=me. I'm going to leave it as-is, on the grounds that it all gets removed a few patches up anyway.
Assignee | ||
Comment 34•14 years ago
|
||
(In reply to comment #26) > Comment on attachment 467668 [details] [diff] [review] > part 11 - v1 - make RasterImage carry a pointer to a Decoder > > Aren't there some includes we could remove in this patch? There aren't - imgIDecoder was already forward-declared in RasterImage.h, and we need the include in RasterImage.cpp for Init/Close flags.
Assignee | ||
Comment 35•14 years ago
|
||
(In reply to comment #27) > > I'm a bit concerned that PostSize doesn't check result values; if, for example, > OnStartContainer returns an error, we will continue decoding now when, in the > past, we would have aborted. I'm not super-concerned about it, though, because > not every decoder did that. The only time imgRequest::OnStartContainer ever returns anything but NS_OK is when it's passed a null image, which presumably we can avoid.
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #30) > This now means we can never reuse decoders. That's fine, but we should make > sure it a) isn't done anywhere and b) is documented as such. (I am concerned > about multipart/x-mixed-replace wrt part a.) We definitely don't do this anywhere. Documenting.
Assignee | ||
Comment 37•14 years ago
|
||
Addressed joe's review comments.
Attachment #467660 -
Attachment is obsolete: true
Assignee | ||
Comment 38•14 years ago
|
||
Addressed joe's review comments
Attachment #467666 -
Attachment is obsolete: true
Assignee | ||
Comment 39•14 years ago
|
||
Addressed joe's review comments.
Attachment #467671 -
Attachment is obsolete: true
Assignee | ||
Comment 40•14 years ago
|
||
Addressed joe's review comments.
Attachment #467672 -
Attachment is obsolete: true
Assignee | ||
Comment 41•14 years ago
|
||
Addressed joe's review comments.
Attachment #467673 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #467993 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #467994 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #467995 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #467996 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #467997 -
Flags: review+
Assignee | ||
Comment 42•14 years ago
|
||
Comment on attachment 467674 [details] [diff] [review] part 17 - v1 - remove imgIDecoder Flagging vlad for sr on the final patch. Vlad, see comment 7 for an explanation of why we're doing this.
Attachment #467674 -
Flags: superreview?(vladimir)
(In reply to comment #33) > (In reply to comment #32) > > Nit: remove DECODERS entirely here and list the directories explicitly on DIRS. > > > > r=me. > > I'm going to leave it as-is, on the grounds that it all gets removed a few > patches up anyway. sgtm
Comment on attachment 467674 [details] [diff] [review] part 17 - v1 - remove imgIDecoder sr=shaver. we may yet need a pluggable-decoder story, but we shouldn't pretend that we really have one now, or carry the cost of the incomplete story we had before. well-played, sir.
Attachment #467674 -
Flags: superreview?(vladimir) → superreview+
Assignee | ||
Comment 45•14 years ago
|
||
Pushed to mozilla-central: part 1: http://hg.mozilla.org/mozilla-central/rev/ba1660575209 part 2: http://hg.mozilla.org/mozilla-central/rev/bdaed1b28b38 part 3: http://hg.mozilla.org/mozilla-central/rev/724bf8e536e5 part 4: http://hg.mozilla.org/mozilla-central/rev/770fef8fe3d0 part 5: http://hg.mozilla.org/mozilla-central/rev/1e72e07b7275 part 6: http://hg.mozilla.org/mozilla-central/rev/5d0e391185c1 part 7: http://hg.mozilla.org/mozilla-central/rev/c57e6d84f5cf part 8: http://hg.mozilla.org/mozilla-central/rev/01d21211af66 part 9: http://hg.mozilla.org/mozilla-central/rev/1fcf4f540527 part 10: http://hg.mozilla.org/mozilla-central/rev/4b1d06fe0663 part 11: http://hg.mozilla.org/mozilla-central/rev/1dfd9dd5a320 part 12: http://hg.mozilla.org/mozilla-central/rev/5cd8adb0daf5 part 13: http://hg.mozilla.org/mozilla-central/rev/10e2cf6396b9 part 14: http://hg.mozilla.org/mozilla-central/rev/8a6518716520 part 15: http://hg.mozilla.org/mozilla-central/rev/4498499e79be part 16: http://hg.mozilla.org/mozilla-central/rev/e0bd6cb8e2ba part 17: http://hg.mozilla.org/mozilla-central/rev/4aa1d52a2f30 Resolving fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/3542b2bfa001 Backed out due to timeouts: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282467574.1282469946.9641.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282480566.1282483226.22020.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282471771.1282473837.29744.gz etc.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 47•14 years ago
|
||
Note: this also caused various Lk regression (trace malloc leaks) - somewhere around 2MB on Linux (also seen on other platforms, but I've not got time to work out the figures).
Comment 48•14 years ago
|
||
Testing hourly builds both sides of, and the build with this patch I have confirmed that the patch(s) checked in with cset 4aa1d52a2f30 is causing extreme spikes in memory use, upwards of 1gig+ in a few seconds. This we discussed briefly this morning on IRC. STR: Using Win7 x64 build 1. make sure d2d is 'on' 2. load several tabs (for this test I have 12) 3. Mouse over the icon in the taskbar 4. Monitor memory use with the taskmanager Repeating above with d2d 'off' there is no massive spike in memory use.
Assignee | ||
Comment 49•14 years ago
|
||
The memory leakage turns out to be the result of me forgetting to make the superclass destructor virtual. :\ I've attached an updated patch. The test hangage is a more tricky issue. It indeed hung for each of the test runs while this patch-set was on tinderbox, but it also hung once after it was backed out. I spent today installing a 32-bit Win7 VM and building an opt build, but couldn't reproduce the problem. The test failure _seems_ pretty orthogonal to my changes, and the failure also doesn't show up on my recent birch pushes (though it doesn't seem to have shown up before either). So since it's a quiet sunday evening, I optimistically re-pushed this. Time will tell.
Attachment #467663 -
Attachment is obsolete: true
Assignee | ||
Comment 50•14 years ago
|
||
Relanded on mozilla-central: part 1: http://hg.mozilla.org/mozilla-central/rev/4849e7d81c77 part 2: http://hg.mozilla.org/mozilla-central/rev/86c63d01ae51 part 3: http://hg.mozilla.org/mozilla-central/rev/535450b85b37 part 4: http://hg.mozilla.org/mozilla-central/rev/5519fadfdbc1 part 5: http://hg.mozilla.org/mozilla-central/rev/840c96279664 part 6: http://hg.mozilla.org/mozilla-central/rev/f88967465b15 part 7: http://hg.mozilla.org/mozilla-central/rev/80be64318c0d part 8: http://hg.mozilla.org/mozilla-central/rev/001c3d9e35a9 part 9: http://hg.mozilla.org/mozilla-central/rev/ec9868fc1ecc part 10: http://hg.mozilla.org/mozilla-central/rev/512925f1acf6 part 11: http://hg.mozilla.org/mozilla-central/rev/7576132bb7e6 part 12: http://hg.mozilla.org/mozilla-central/rev/c1d8b34e45c7 part 13: http://hg.mozilla.org/mozilla-central/rev/5b2f0911e33e part 14: http://hg.mozilla.org/mozilla-central/rev/27cfdf51e713 part 15: http://hg.mozilla.org/mozilla-central/rev/557bba0d93dd part 16: http://hg.mozilla.org/mozilla-central/rev/ebd6e47046d6 part 17: http://hg.mozilla.org/mozilla-central/rev/3d3fd377d82e This seems to have stuck. The leak is fixed, and the Win7opt-Moth timeout seems to have gone away. What a great way to spend a sunday. Resolving fixed. Whew.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 51•14 years ago
|
||
I don't know if its something to worry about, but I can still repo the memory spike with the updated patch and re-landing. I currently have 11 tabs open: MSNBC, CNN, MZ forum, http://ftp hourly builds, tbpl, digg.com, neowin.net Landing que page, and 3 bugzilla pages. On startup of the browser I let the UI stabilize, and the throbbers on the tabs stop spinning. I then immediately go to the Minefield icon in the taskbar and mouse over to activate 'aero-peek', (taskbar previews) - then watching the taskmanager I see the working memory, and peak memory start to climb with the peak working hitting over 1gig, last start up with this procedure hit 1.4gig. After a a min or two, working falls to what is somewhat 'normal' for my tabs, around 250meg-300meg. So far, I've not noticed any performance or browser issues but I've not let it run for any extended period of time, trying to verify what I'm seeing. If it start the browser with the tabs listed, and wait for 2-3 minutes before mousing over the taskbar icon to activate the previews - the memory jumps some, but anywhere near the levels seen when activating the previews immediately after startup. Turning off d2d again resolves the huge spike in memory use problem.
Assignee | ||
Comment 52•14 years ago
|
||
(In reply to comment #51) > I don't know if its something to worry about, but I can still repo the memory > spike with the updated patch and re-landing. > > I currently have 11 tabs open: > MSNBC, CNN, MZ forum, http://ftp hourly builds, tbpl, digg.com, neowin.net > Landing que page, and 3 bugzilla pages. > > On startup of the browser I let the UI stabilize, and the throbbers on the tabs > stop spinning. I then immediately go to the Minefield icon in the taskbar and > mouse over to activate 'aero-peek', (taskbar previews) - then watching the > taskmanager I see the working memory, and peak memory start to climb with the > peak working hitting over 1gig, last start up with this procedure hit 1.4gig. > After a a min or two, working falls to what is somewhat 'normal' for my tabs, > around 250meg-300meg. > > So far, I've not noticed any performance or browser issues but I've not let it > run for any extended period of time, trying to verify what I'm seeing. > > If it start the browser with the tabs listed, and wait for 2-3 minutes before > mousing over the taskbar icon to activate the previews - the memory jumps some, > but anywhere near the levels seen when activating the previews immediately > after startup. > > Turning off d2d again resolves the huge spike in memory use problem. Sorry for landing without resolving this - I couldn't reproduce it on my VM, so I assumed it'd been fixed with the leak. Still, if you're seeing different behavior around this patch with d2d on/off, we should look into it for sure. I still can't seem to reproduce it, even with your exact tab set. At the same time, D2D doesn't seem to work all that great on my VM. This patch really shouldn't change any behavior outside of imagelib, so I'd be really curious to see what's going on. Can you file a bug, CC me and Bas, and include about:memory information? Thanks for your help.
Comment 53•14 years ago
|
||
If I udnerstand right, imgDecoder interface was intended to make a new image decoder easier to adding onto Gecko, as an extension. On the other hand, <Decoder.h> and <RasterImage.h> do not seems capsuling internal libraries and yet they're not exposed as "public" headers. It seems to me that Bobby have notice this issue since last year. https://bugzilla.mozilla.org/show_bug.cgi?id=36351#c121 I failed to find any follow-up for switching decoders (XPCOM components) based on MIME types. Can users write a decoder extension, OR they should hack libpr0n directly?
Assignee | ||
Comment 54•14 years ago
|
||
(In reply to comment #53) > I failed to find any follow-up for switching decoders (XPCOM components) based > on MIME types. Can users write a decoder extension, OR they should hack libpr0n > directly? As of this bug we no longer support plugable decoders. You'll have to hack libpr0n directly (though it should still be very modular and easy to do, and in fact easier than it used to be thanks to the superclass). What kind of extension were you planning to write?
Comment 55•14 years ago
|
||
(In reply to comment #54) > As of this bug we no longer support plugable decoders. You'll have to hack > libpr0n directly (though it should still be very modular and easy to do, and in > fact easier than it used to be thanks to the superclass). I see. And I also noticed the new framework is by far better than the previous one, in terms of hacking libpr0n. > What kind of extension were you planning to write? What's in my mind were MNG and JPEG 2000. I totally agree that bug 36351 etc. are WONTFIX. Apparently, normal users don't need them, and, in my opinion, these formats should fade away in the long run, at least from WWW. However, these bugs are a bit flaming, you know. That means there exists a little abnormal users somewhere in this world. I really wonder why those folks haven't yet developed an extension using imgLoader so far. But I slightly feel sympathy with them, because they've lost such chance, and now they have to use a hacky version of Mozilla browser (if it exists), instead of official Firefox.
Assignee | ||
Comment 56•14 years ago
|
||
(In reply to comment #55) > > I see. And I also noticed the new framework is by far better than the previous > one, in terms of hacking libpr0n. > Thanks! ;-) > What's in my mind were MNG and JPEG 2000. I totally agree that bug 36351 etc. > are WONTFIX. Apparently, normal users don't need them, and, in my opinion, > these formats should fade away in the long run, at least from WWW. However, > these bugs are a bit flaming, you know. That means there exists a little > abnormal users somewhere in this world. > > I really wonder why those folks haven't yet developed an extension using > imgLoader so far. But I slightly feel sympathy with them, because they've lost > such chance, and now they have to use a hacky version of Mozilla browser (if it > exists), instead of official Firefox. There was actually a MNG extension back in the day (mngzilla), but it hasn't been updated since 2007. Since the APIs also changed for Firefox 3.6, this means the extension has been broken since the release of 3.6 and nobody noticed.
Comment 57•14 years ago
|
||
I never updated the mngzilla or better created an image extension as this all was blocked by other bugs (like bug 374089 ). Also flames and WONTFIXES of bugs from some mozilla developers (mainly the APNG libpr0n) got me to reject my work. There is no image extension as it wasn't cleanly possible.
You need to log in
before you can comment on or make changes to this bug.
Description
•