From ce836868f3183934cd8739c41131ddbc6343b72c Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Mon, 26 Oct 2015 14:31:12 -0400 Subject: [PATCH] Bug 1218488 - clarify buffer ownership for nsICanvasRenderingContextInternal::GetBuffer; r=Bas,baku This patch started life as making ImageEncoder.cpp:EncodingRunnable not use nsAutoArrayPtr, but the API effects rippled out from there. On the whole, I think using UniquePtr throughout has made the code clearer. --- dom/base/ImageEncoder.cpp | 14 ++++++------- dom/base/ImageEncoder.h | 3 ++- dom/canvas/CanvasRenderingContext2D.cpp | 20 ++++++++---------- dom/canvas/CanvasRenderingContext2D.h | 3 ++- dom/canvas/CanvasRenderingContextHelper.cpp | 7 ++++--- dom/canvas/WebGLContext.cpp | 12 +++++------ dom/canvas/WebGLContext.h | 3 +-- .../nsICanvasRenderingContextInternal.h | 3 ++- gfx/2d/DataSurfaceHelpers.cpp | 10 ++++----- gfx/2d/DataSurfaceHelpers.h | 8 +++---- gfx/thebes/gfxUtils.cpp | 21 ++++++++----------- gfx/thebes/gfxUtils.h | 8 +++---- widget/windows/WinUtils.cpp | 10 ++++----- widget/windows/WinUtils.h | 7 ++++--- widget/windows/nsWindowGfx.cpp | 5 +++-- 15 files changed, 67 insertions(+), 67 deletions(-) diff --git a/dom/base/ImageEncoder.cpp b/dom/base/ImageEncoder.cpp index a723f91b1254..fd203599d462 100644 --- a/dom/base/ImageEncoder.cpp +++ b/dom/base/ImageEncoder.cpp @@ -138,7 +138,7 @@ public: EncodingRunnable(const nsAString& aType, const nsAString& aOptions, - uint8_t* aImageBuffer, + UniquePtr aImageBuffer, layers::Image* aImage, imgIEncoder* aEncoder, EncodingCompleteEvent* aEncodingCompleteEvent, @@ -147,7 +147,7 @@ public: bool aUsingCustomOptions) : mType(aType) , mOptions(aOptions) - , mImageBuffer(aImageBuffer) + , mImageBuffer(Move(aImageBuffer)) , mImage(aImage) , mEncoder(aEncoder) , mEncodingCompleteEvent(aEncodingCompleteEvent) @@ -161,7 +161,7 @@ public: nsCOMPtr stream; nsresult rv = ImageEncoder::ExtractDataInternal(mType, mOptions, - mImageBuffer, + mImageBuffer.get(), mFormat, mSize, mImage, @@ -175,7 +175,7 @@ public: if (rv == NS_ERROR_INVALID_ARG && mUsingCustomOptions) { rv = ImageEncoder::ExtractDataInternal(mType, EmptyString(), - mImageBuffer, + mImageBuffer.get(), mFormat, mSize, mImage, @@ -220,7 +220,7 @@ public: private: nsAutoString mType; nsAutoString mOptions; - nsAutoArrayPtr mImageBuffer; + UniquePtr mImageBuffer; RefPtr mImage; nsCOMPtr mEncoder; RefPtr mEncodingCompleteEvent; @@ -287,7 +287,7 @@ nsresult ImageEncoder::ExtractDataAsync(nsAString& aType, const nsAString& aOptions, bool aUsingCustomOptions, - uint8_t* aImageBuffer, + UniquePtr aImageBuffer, int32_t aFormat, const nsIntSize aSize, EncodeCompleteCallback* aEncodeCallback) @@ -306,7 +306,7 @@ ImageEncoder::ExtractDataAsync(nsAString& aType, nsCOMPtr event = new EncodingRunnable(aType, aOptions, - aImageBuffer, + Move(aImageBuffer), nullptr, encoder, completeEvent, diff --git a/dom/base/ImageEncoder.h b/dom/base/ImageEncoder.h index a38bbd209140..def44c547f40 100644 --- a/dom/base/ImageEncoder.h +++ b/dom/base/ImageEncoder.h @@ -11,6 +11,7 @@ #include "nsError.h" #include "mozilla/dom/File.h" #include "mozilla/dom/HTMLCanvasElementBinding.h" +#include "mozilla/UniquePtr.h" #include "nsLayoutUtils.h" #include "nsSize.h" @@ -58,7 +59,7 @@ public: static nsresult ExtractDataAsync(nsAString& aType, const nsAString& aOptions, bool aUsingCustomOptions, - uint8_t* aImageBuffer, + UniquePtr aImageBuffer, int32_t aFormat, const nsIntSize aSize, EncodeCompleteCallback* aEncodeCallback); diff --git a/dom/canvas/CanvasRenderingContext2D.cpp b/dom/canvas/CanvasRenderingContext2D.cpp index 112142e867de..47e15a9aa904 100644 --- a/dom/canvas/CanvasRenderingContext2D.cpp +++ b/dom/canvas/CanvasRenderingContext2D.cpp @@ -1646,26 +1646,24 @@ CanvasRenderingContext2D::SetContextOptions(JSContext* aCx, return NS_OK; } -void -CanvasRenderingContext2D::GetImageBuffer(uint8_t** aImageBuffer, - int32_t* aFormat) +UniquePtr +CanvasRenderingContext2D::GetImageBuffer(int32_t* aFormat) { - *aImageBuffer = nullptr; *aFormat = 0; EnsureTarget(); RefPtr snapshot = mTarget->Snapshot(); if (!snapshot) { - return; + return nullptr; } RefPtr data = snapshot->GetDataSurface(); if (!data || data->GetSize() != IntSize(mWidth, mHeight)) { - return; + return nullptr; } - *aImageBuffer = SurfaceToPackedBGRA(data); *aFormat = imgIEncoder::INPUT_FORMAT_HOSTARGB; + return SurfaceToPackedBGRA(data); } nsString CanvasRenderingContext2D::GetHitRegion(const mozilla::gfx::Point& aPoint) @@ -1691,15 +1689,15 @@ CanvasRenderingContext2D::GetInputStream(const char *aMimeType, return NS_ERROR_FAILURE; } - nsAutoArrayPtr imageBuffer; int32_t format = 0; - GetImageBuffer(getter_Transfers(imageBuffer), &format); + UniquePtr imageBuffer = GetImageBuffer(&format); if (!imageBuffer) { return NS_ERROR_FAILURE; } - return ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer, format, - encoder, aEncoderOptions, aStream); + return ImageEncoder::GetInputStream(mWidth, mHeight, imageBuffer.get(), + format, encoder, aEncoderOptions, + aStream); } SurfaceFormat diff --git a/dom/canvas/CanvasRenderingContext2D.h b/dom/canvas/CanvasRenderingContext2D.h index add5bb53bd99..9264e356a8e4 100644 --- a/dom/canvas/CanvasRenderingContext2D.h +++ b/dom/canvas/CanvasRenderingContext2D.h @@ -20,6 +20,7 @@ #include "mozilla/dom/CanvasPattern.h" #include "mozilla/gfx/Rect.h" #include "mozilla/gfx/2D.h" +#include "mozilla/UniquePtr.h" #include "gfx2DGlue.h" #include "imgIEncoder.h" #include "nsLayoutUtils.h" @@ -526,7 +527,7 @@ public: friend class CanvasRenderingContext2DUserData; - virtual void GetImageBuffer(uint8_t** aImageBuffer, int32_t* aFormat) override; + virtual UniquePtr GetImageBuffer(int32_t* aFormat) override; // Given a point, return hit region ID if it exists diff --git a/dom/canvas/CanvasRenderingContextHelper.cpp b/dom/canvas/CanvasRenderingContextHelper.cpp index e050f0e56fc2..ac4b87c96134 100644 --- a/dom/canvas/CanvasRenderingContextHelper.cpp +++ b/dom/canvas/CanvasRenderingContextHelper.cpp @@ -7,6 +7,7 @@ #include "ImageEncoder.h" #include "mozilla/dom/CanvasRenderingContext2D.h" #include "mozilla/Telemetry.h" +#include "mozilla/UniquePtr.h" #include "nsContentUtils.h" #include "nsDOMJSUtils.h" #include "nsIScriptContext.h" @@ -49,10 +50,10 @@ CanvasRenderingContextHelper::ToBlob(JSContext* aCx, } } - uint8_t* imageBuffer = nullptr; + UniquePtr imageBuffer; int32_t format = 0; if (mCurrentContext) { - mCurrentContext->GetImageBuffer(&imageBuffer, &format); + imageBuffer = mCurrentContext->GetImageBuffer(&format); } // Encoder callback when encoding is complete. @@ -99,7 +100,7 @@ CanvasRenderingContextHelper::ToBlob(JSContext* aCx, aRv = ImageEncoder::ExtractDataAsync(type, params, usingCustomParseOptions, - imageBuffer, + Move(imageBuffer), format, GetWidthHeight(), callback); diff --git a/dom/canvas/WebGLContext.cpp b/dom/canvas/WebGLContext.cpp index 762e47f7412d..7a712f623926 100644 --- a/dom/canvas/WebGLContext.cpp +++ b/dom/canvas/WebGLContext.cpp @@ -1052,25 +1052,25 @@ WebGLContext::LoseOldestWebGLContextIfLimitExceeded() } } -void -WebGLContext::GetImageBuffer(uint8_t** out_imageBuffer, int32_t* out_format) +UniquePtr +WebGLContext::GetImageBuffer(int32_t* out_format) { - *out_imageBuffer = nullptr; *out_format = 0; // Use GetSurfaceSnapshot() to make sure that appropriate y-flip gets applied bool premult; RefPtr snapshot = GetSurfaceSnapshot(mOptions.premultipliedAlpha ? nullptr : &premult); - if (!snapshot) - return; + if (!snapshot) { + return nullptr; + } MOZ_ASSERT(mOptions.premultipliedAlpha || !premult, "We must get unpremult when we ask for it!"); RefPtr dataSurface = snapshot->GetDataSurface(); return gfxUtils::GetImageBuffer(dataSurface, mOptions.premultipliedAlpha, - out_imageBuffer, out_format); + out_format); } NS_IMETHODIMP diff --git a/dom/canvas/WebGLContext.h b/dom/canvas/WebGLContext.h index 171d89208c83..134672d05770 100644 --- a/dom/canvas/WebGLContext.h +++ b/dom/canvas/WebGLContext.h @@ -237,8 +237,7 @@ public: return NS_ERROR_NOT_IMPLEMENTED; } - virtual void GetImageBuffer(uint8_t** out_imageBuffer, - int32_t* out_format) override; + virtual UniquePtr GetImageBuffer(int32_t* out_format) override; NS_IMETHOD GetInputStream(const char* mimeType, const char16_t* encoderOptions, nsIInputStream** out_stream) override; diff --git a/dom/canvas/nsICanvasRenderingContextInternal.h b/dom/canvas/nsICanvasRenderingContextInternal.h index b5212dd9e25a..a18e4001bb11 100644 --- a/dom/canvas/nsICanvasRenderingContextInternal.h +++ b/dom/canvas/nsICanvasRenderingContextInternal.h @@ -14,6 +14,7 @@ #include "mozilla/dom/HTMLCanvasElement.h" #include "mozilla/dom/OffscreenCanvas.h" #include "mozilla/RefPtr.h" +#include "mozilla/UniquePtr.h" #define NS_ICANVASRENDERINGCONTEXTINTERNAL_IID \ { 0xb84f2fed, 0x9d4b, 0x430b, \ @@ -96,7 +97,7 @@ public: NS_IMETHOD InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, int32_t width, int32_t height) = 0; // Creates an image buffer. Returns null on failure. - virtual void GetImageBuffer(uint8_t** imageBuffer, int32_t* format) = 0; + virtual mozilla::UniquePtr GetImageBuffer(int32_t* format) = 0; // Gives you a stream containing the image represented by this context. // The format is given in mimeTime, for example "image/png". diff --git a/gfx/2d/DataSurfaceHelpers.cpp b/gfx/2d/DataSurfaceHelpers.cpp index 6776db243974..965e6ead64cd 100644 --- a/gfx/2d/DataSurfaceHelpers.cpp +++ b/gfx/2d/DataSurfaceHelpers.cpp @@ -110,7 +110,7 @@ CopyBGRXSurfaceDataToPackedBGRArray(uint8_t* aSrc, uint8_t* aDst, } } -uint8_t* +UniquePtr SurfaceToPackedBGRA(DataSourceSurface *aSurface) { SurfaceFormat format = aSurface->GetFormat(); @@ -120,25 +120,25 @@ SurfaceToPackedBGRA(DataSourceSurface *aSurface) IntSize size = aSurface->GetSize(); - uint8_t* imageBuffer = new (std::nothrow) uint8_t[size.width * size.height * sizeof(uint32_t)]; + UniquePtr imageBuffer( + new (std::nothrow) uint8_t[size.width * size.height * sizeof(uint32_t)]); if (!imageBuffer) { return nullptr; } DataSourceSurface::MappedSurface map; if (!aSurface->Map(DataSourceSurface::MapType::READ, &map)) { - delete [] imageBuffer; return nullptr; } - CopySurfaceDataToPackedArray(map.mData, imageBuffer, size, + CopySurfaceDataToPackedArray(map.mData, imageBuffer.get(), size, map.mStride, 4 * sizeof(uint8_t)); aSurface->Unmap(); if (format == SurfaceFormat::B8G8R8X8) { // Convert BGRX to BGRA by setting a to 255. - ConvertBGRXToBGRA(reinterpret_cast(imageBuffer), size, size.width * sizeof(uint32_t)); + ConvertBGRXToBGRA(imageBuffer.get(), size, size.width * sizeof(uint32_t)); } return imageBuffer; diff --git a/gfx/2d/DataSurfaceHelpers.h b/gfx/2d/DataSurfaceHelpers.h index 40a4db5b3ef0..6cd837d64b97 100644 --- a/gfx/2d/DataSurfaceHelpers.h +++ b/gfx/2d/DataSurfaceHelpers.h @@ -8,6 +8,8 @@ #include "2D.h" +#include "mozilla/UniquePtr.h" + namespace mozilla { namespace gfx { @@ -25,11 +27,9 @@ CopySurfaceDataToPackedArray(uint8_t* aSrc, uint8_t* aDst, IntSize aSrcSize, int32_t aSrcStride, int32_t aBytesPerPixel); /** - * Convert aSurface to a packed buffer in BGRA format. The pixel data is - * returned in a buffer allocated with new uint8_t[]. The caller then has - * ownership of the buffer and is responsible for delete[]'ing it. + * Convert aSurface to a packed buffer in BGRA format. */ -uint8_t* +UniquePtr SurfaceToPackedBGRA(DataSourceSurface *aSurface); /** diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp index 61e4ef50cd57..ec66f494898d 100644 --- a/gfx/thebes/gfxUtils.cpp +++ b/gfx/thebes/gfxUtils.cpp @@ -1547,26 +1547,24 @@ gfxUtils::CopyAsDataURI(DrawTarget* aDT) } } -/* static */ void +/* static */ UniquePtr gfxUtils::GetImageBuffer(gfx::DataSourceSurface* aSurface, bool aIsAlphaPremultiplied, - uint8_t** outImageBuffer, int32_t* outFormat) { - *outImageBuffer = nullptr; *outFormat = 0; DataSourceSurface::MappedSurface map; if (!aSurface->Map(DataSourceSurface::MapType::READ, &map)) - return; + return nullptr; uint32_t bufferSize = aSurface->GetSize().width * aSurface->GetSize().height * 4; - uint8_t* imageBuffer = new (fallible) uint8_t[bufferSize]; + UniquePtr imageBuffer(new (fallible) uint8_t[bufferSize]); if (!imageBuffer) { aSurface->Unmap(); - return; + return nullptr; } - memcpy(imageBuffer, map.mData, bufferSize); + memcpy(imageBuffer.get(), map.mData, bufferSize); aSurface->Unmap(); @@ -1577,12 +1575,12 @@ gfxUtils::GetImageBuffer(gfx::DataSourceSurface* aSurface, // Yes, it is THAT silly. // Except for different lossy conversions by color, // we could probably just change the label, and not change the data. - gfxUtils::ConvertBGRAtoRGBA(imageBuffer, bufferSize); + gfxUtils::ConvertBGRAtoRGBA(imageBuffer.get(), bufferSize); format = imgIEncoder::INPUT_FORMAT_RGBA; } - *outImageBuffer = imageBuffer; *outFormat = format; + return imageBuffer; } /* static */ nsresult @@ -1598,15 +1596,14 @@ gfxUtils::GetInputStream(gfx::DataSourceSurface* aSurface, if (!encoder) return NS_ERROR_FAILURE; - nsAutoArrayPtr imageBuffer; int32_t format = 0; - GetImageBuffer(aSurface, aIsAlphaPremultiplied, getter_Transfers(imageBuffer), &format); + UniquePtr imageBuffer = GetImageBuffer(aSurface, aIsAlphaPremultiplied, &format); if (!imageBuffer) return NS_ERROR_FAILURE; return dom::ImageEncoder::GetInputStream(aSurface->GetSize().width, aSurface->GetSize().height, - imageBuffer, format, + imageBuffer.get(), format, encoder, aEncoderOptions, outStream); } diff --git a/gfx/thebes/gfxUtils.h b/gfx/thebes/gfxUtils.h index 9f6a7ca179f9..08baf4c74089 100644 --- a/gfx/thebes/gfxUtils.h +++ b/gfx/thebes/gfxUtils.h @@ -10,6 +10,7 @@ #include "imgIContainer.h" #include "mozilla/gfx/2D.h" #include "mozilla/RefPtr.h" +#include "mozilla/UniquePtr.h" #include "nsColor.h" #include "nsPrintfCString.h" #include "mozilla/gfx/Rect.h" @@ -282,10 +283,9 @@ public: static nsCString GetAsDataURI(DrawTarget* aDT); static nsCString GetAsLZ4Base64Str(DataSourceSurface* aSourceSurface); - static void GetImageBuffer(DataSourceSurface* aSurface, - bool aIsAlphaPremultiplied, - uint8_t** outImageBuffer, - int32_t* outFormat); + static mozilla::UniquePtr GetImageBuffer(DataSourceSurface* aSurface, + bool aIsAlphaPremultiplied, + int32_t* outFormat); static nsresult GetInputStream(DataSourceSurface* aSurface, bool aIsAlphaPremultiplied, diff --git a/widget/windows/WinUtils.cpp b/widget/windows/WinUtils.cpp index bb8517066817..b5ccee1baaf6 100644 --- a/widget/windows/WinUtils.cpp +++ b/widget/windows/WinUtils.cpp @@ -1212,7 +1212,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI, // Allocate a new buffer that we own and can use out of line in // another thread. - uint8_t *data = SurfaceToPackedBGRA(dataSurface); + UniquePtr data = SurfaceToPackedBGRA(dataSurface); if (!data) { return NS_ERROR_OUT_OF_MEMORY; } @@ -1220,7 +1220,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI, int32_t dataLength = stride * size.height; // AsyncEncodeAndWriteIcon takes ownership of the heap allocated buffer - nsCOMPtr event = new AsyncEncodeAndWriteIcon(path, data, + nsCOMPtr event = new AsyncEncodeAndWriteIcon(path, Move(data), dataLength, stride, size.width, @@ -1234,7 +1234,7 @@ AsyncFaviconDataReady::OnComplete(nsIURI *aFaviconURI, // Warning: AsyncEncodeAndWriteIcon assumes ownership of the aData buffer passed in AsyncEncodeAndWriteIcon::AsyncEncodeAndWriteIcon(const nsAString &aIconPath, - uint8_t *aBuffer, + UniquePtr aBuffer, uint32_t aBufferLength, uint32_t aStride, uint32_t aWidth, @@ -1242,7 +1242,7 @@ AsyncEncodeAndWriteIcon::AsyncEncodeAndWriteIcon(const nsAString &aIconPath, const bool aURLShortcut) : mURLShortcut(aURLShortcut), mIconPath(aIconPath), - mBuffer(aBuffer), + mBuffer(Move(aBuffer)), mBufferLength(aBufferLength), mStride(aStride), mWidth(aWidth), @@ -1257,7 +1257,7 @@ NS_IMETHODIMP AsyncEncodeAndWriteIcon::Run() // Note that since we're off the main thread we can't use // gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget() RefPtr surface = - Factory::CreateWrappingDataSourceSurface(mBuffer, mStride, + Factory::CreateWrappingDataSourceSurface(mBuffer.get(), mStride, IntSize(mWidth, mHeight), SurfaceFormat::B8G8R8A8); diff --git a/widget/windows/WinUtils.h b/widget/windows/WinUtils.h index 8cce2e7176e0..20ee87f36b2f 100644 --- a/widget/windows/WinUtils.h +++ b/widget/windows/WinUtils.h @@ -36,6 +36,7 @@ #include "mozilla/Attributes.h" #include "mozilla/EventForwards.h" +#include "mozilla/UniquePtr.h" /** * NS_INLINE_DECL_IUNKNOWN_REFCOUNTING should be used for defining and @@ -459,15 +460,15 @@ public: // Warning: AsyncEncodeAndWriteIcon assumes ownership of the aData buffer passed in AsyncEncodeAndWriteIcon(const nsAString &aIconPath, - uint8_t *aData, uint32_t aDataLen, uint32_t aStride, - uint32_t aWidth, uint32_t aHeight, + UniquePtr aData, uint32_t aDataLen, + uint32_t aStride, uint32_t aWidth, uint32_t aHeight, const bool aURLShortcut); private: virtual ~AsyncEncodeAndWriteIcon(); nsAutoString mIconPath; - nsAutoArrayPtr mBuffer; + UniquePtr mBuffer; HMODULE sDwmDLL; uint32_t mBufferLength; uint32_t mStride; diff --git a/widget/windows/nsWindowGfx.cpp b/widget/windows/nsWindowGfx.cpp index 28f0cc53fe77..9c173d195bba 100644 --- a/widget/windows/nsWindowGfx.cpp +++ b/widget/windows/nsWindowGfx.cpp @@ -640,7 +640,7 @@ nsresult nsWindowGfx::CreateIcon(imgIContainer *aContainer, MOZ_ASSERT(dataSurface->GetFormat() == SurfaceFormat::B8G8R8A8); uint8_t* data = nullptr; - nsAutoArrayPtr autoDeleteArray; + UniquePtr autoDeleteArray; if (map.mStride == BytesPerPixel(dataSurface->GetFormat()) * iconSize.width) { // Mapped data is already packed data = map.mData; @@ -653,7 +653,8 @@ nsresult nsWindowGfx::CreateIcon(imgIContainer *aContainer, dataSurface->Unmap(); map.mData = nullptr; - data = autoDeleteArray = SurfaceToPackedBGRA(dataSurface); + autoDeleteArray = SurfaceToPackedBGRA(dataSurface); + data = autoDeleteArray.get(); NS_ENSURE_TRUE(data, NS_ERROR_FAILURE); }