Closed Bug 513681 Opened 10 years ago Closed 9 years ago

Eliminate repeated code with image decoder superclass

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(17 files, 6 obsolete files)

1.80 KB, patch
joe
: review+
Details | Diff | Splinter Review
8.69 KB, patch
joe
: review+
khuey
: review+
Details | Diff | Splinter Review
17.21 KB, patch
joe
: review+
Details | Diff | Splinter Review
18.53 KB, patch
joe
: review+
Details | Diff | Splinter Review
8.62 KB, patch
joe
: review+
Details | Diff | Splinter Review
3.36 KB, patch
joe
: review+
Details | Diff | Splinter Review
27.40 KB, patch
joe
: review+
Details | Diff | Splinter Review
1.19 KB, patch
joe
: review+
Details | Diff | Splinter Review
1.80 KB, patch
joe
: review+
Details | Diff | Splinter Review
21.73 KB, patch
joe
: review+
Details | Diff | Splinter Review
9.57 KB, patch
joe
: review+
shaver
: superreview+
Details | Diff | Splinter Review
6.17 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.16 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.84 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.53 KB, patch
bholley
: review+
Details | Diff | Splinter Review
15.27 KB, patch
bholley
: review+
Details | Diff | Splinter Review
9.60 KB, patch
Details | Diff | Splinter Review
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.
Summary: Eliminated repeated code with image decoder superclass → Eliminate repeated code with image decoder superclass
this should be the "right fix" to bug 516265.
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.
Blocks: 511899
Blocks: 514033
nominating - blocks a blocker.
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
blocking2.0: --- → ?
blocking2.0: ? → beta5+
Depends on: 587371
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.
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.
We're deCOM-ing image decoders, so we can't instantiate them with do_CreateInstance anymore. Use a simple switch statement instead.
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.
This patch moves the decoders out of their subdirectories. They're annoying to deal with in subdirectories, and gecko is moving towards flat-ness.
This patch introduces the decoder superclass that all the decoders will eventually inherit from.
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.
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().
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.
This patch makes each of the decoders inherit from the superclass, and moves various bits of common functionality into the superclass.
Now that all the decoders implement mozilla::imagelib::Decoder, RasterImage can know that it's holding onto something more concrete than an imgIDecoder.
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.
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".
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.
Similar to the previous patch, this patch moves the logic of beginning and ending frames into the superclass.
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().
As the crowning achievement of this stack, this final patch eliminates imgIDecoder. Woohoo!
Attachment #467658 - Flags: review?(joe)
Attachment #467659 - Flags: review?(me)
Attachment #467659 - Flags: review?(joe)
Attachment #467660 - Flags: review?(joe)
Attachment #467661 - Flags: review?(joe)
Attachment #467662 - Flags: review?(joe)
Attachment #467663 - Flags: review?(joe)
Attachment #467664 - Flags: review?(joe)
Attachment #467665 - Flags: review?(joe)
Attachment #467666 - Flags: review?(joe)
Attachment #467667 - Flags: review?(joe)
Attachment #467668 - Flags: review?(joe)
Attachment #467669 - Flags: review?(joe)
Attachment #467670 - Flags: review?(joe)
Attachment #467671 - Flags: review?(joe)
Attachment #467672 - Flags: review?(joe)
Attachment #467673 - Flags: review?(joe)
Attachment #467674 - Flags: review?(joe)
Attachment #467658 - Flags: review?(joe) → review+
Attachment #467659 - Flags: review?(joe) → review+
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 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+
Attachment #467662 - Flags: review?(joe) → review+
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+
Attachment #467664 - Flags: review?(joe) → review+
Attachment #467665 - Flags: review?(joe) → review+
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+
(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?
Attachment #467667 - Flags: review?(joe) → review+
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+
Attachment #467669 - Flags: review?(joe) → review+
Attachment #467670 - Flags: review?(joe) → review+
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+
(In reply to comment #25)
> (In reply to comment #21)
> 
> NS_NOTREACHED?

NS_NOTREACHED is, unfortunately, not fatal at the moment.
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 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 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+
(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.
(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.
(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.
(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.
Addressed joe's review comments.
Attachment #467660 - Attachment is obsolete: true
Addressed joe's review comments
Attachment #467666 - Attachment is obsolete: true
Addressed joe's review comments.
Attachment #467671 - Attachment is obsolete: true
Addressed joe's review comments.
Attachment #467672 - Attachment is obsolete: true
Attachment #467993 - Flags: review+
Attachment #467994 - Flags: review+
Attachment #467995 - Flags: review+
Attachment #467996 - Flags: review+
Attachment #467997 - Flags: review+
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+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
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.
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
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.
(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.
No longer depends on: 590100
No longer blocks: 511899
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?
(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?
(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.
(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.
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.
Duplicate of this bug: 577227
Blocks: 922471
You need to log in before you can comment on or make changes to this bug.