nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1503935 - Fix some WebP decoder implementation bugs. r=tnikkel

Browse source 
First we did not handle the SourceBufferIterator::WAITING state which
can happen when we get woken up but there is no data to read from the
SourceBufferIterator. StreamingLexer handled this properly by yielding
with NEED_MORE_DATA, and properly scheduling the decoder to resume. This
patch does the same in the WebP decoder.

Second nsWebPDecoder::GetType was not implemented. This meant it would
return DecoderType::UNKNOWN, and would fail to recreate the decoder if
we are discarding frames and need to restart from the beginning. In
addition to implementing that method, this patch also corrects an assert
in DecoderFactory::CloneAnimationDecoder which failed to check for WebP
as a supported animated decoder.

This patch also modestly improves the logging output and library method
checks.

Differential Revision: https://phabricator.services.mozilla.com/D10624
This commit is contained in:
Andrew Osmond 2018-11-01 16:36:16 -04:00
parent e471e325c8
commit 414919edbf
10 changed files with 167 additions and 26 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

3
image/DecoderFactory.cpp
View file

@ -246,7 +246,8 @@ DecoderFactory::CloneAnimationDecoder(Decoder* aDecoder)
// get scheduled yet, or it has only decoded the first frame and has yet to
// rediscover it is animated).
DecoderType type = aDecoder->GetType();
MOZ_ASSERT(type == DecoderType::GIF || type == DecoderType::PNG,
MOZ_ASSERT(type == DecoderType::GIF || type == DecoderType::PNG ||
type == DecoderType::WEBP,
"Calling CloneAnimationDecoder for non-animating DecoderType");
RefPtr<Decoder> decoder = GetDecoder(type, nullptr, /* aIsRedecode = */ true);

19
image/IDecodingTask.cpp
View file

@ -185,8 +185,10 @@ MetadataDecodingTask::Run()
// AnonymousDecodingTask implementation.
///////////////////////////////////////////////////////////////////////////////
AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder)
AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder,
bool aResumable)
: mDecoder(aDecoder)
, mResumable(aResumable)
{ }
void
@ -211,5 +213,20 @@ AnonymousDecodingTask::Run()
}
}
void
AnonymousDecodingTask::Resume()
{
// Anonymous decoders normally get all their data at once. We have tests
// where they don't; typically in these situations, the test re-runs them
// manually. However some tests want to verify Resume works, so they will
// explicitly request this behaviour.
if (mResumable) {
RefPtr<AnonymousDecodingTask> self(this);
NS_DispatchToMainThread(NS_NewRunnableFunction(
"image::AnonymousDecodingTask::Resume",
[self]() -> void { self->Run(); }));
}
}
} // namespace image
} // namespace mozilla

9
image/IDecodingTask.h
View file

@ -109,22 +109,21 @@ class AnonymousDecodingTask final : public IDecodingTask
public:
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AnonymousDecodingTask, override)
explicit AnonymousDecodingTask(NotNull<Decoder*> aDecoder);
explicit AnonymousDecodingTask(NotNull<Decoder*> aDecoder,
bool aResumable);
void Run() override;
bool ShouldPreferSyncRun() const override { return true; }
TaskPriority Priority() const override { return TaskPriority::eLow; }
// Anonymous decoders normally get all their data at once. We have tests where
// they don't; in these situations, the test re-runs them manually. So no
// matter what, we don't want to resume by posting a task to the DecodePool.
void Resume() override { }
void Resume() override;
private:
virtual ~AnonymousDecodingTask() { }
NotNull<RefPtr<Decoder>> mDecoder;
bool mResumable;
};
} // namespace image

6
image/ImageOps.cpp
View file

@ -179,7 +179,8 @@ ImageOps::DecodeMetadata(ImageBuffer* aBuffer,
}
// Run the decoder synchronously.
RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
task->Run();
if (!decoder->GetDecodeDone() || decoder->HasError()) {
return NS_ERROR_FAILURE;
@ -233,7 +234,8 @@ ImageOps::DecodeToSurface(ImageBuffer* aBuffer,
}
// Run the decoder synchronously.
RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
task->Run();
if (!decoder->GetDecodeDone() || decoder->HasError()) {
return nullptr;

21
image/decoders/nsWebPDecoder.cpp
View file

@ -110,7 +110,7 @@ nsWebPDecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
SourceBufferIterator::State state = SourceBufferIterator::COMPLETE;
if (!mIteratorComplete) {
state = aIterator.Advance(SIZE_MAX);
state = aIterator.AdvanceOrScheduleResume(SIZE_MAX, aOnResume);
// We need to remember since we can't advance a complete iterator.
mIteratorComplete = state == SourceBufferIterator::COMPLETE;
@ -133,6 +133,8 @@ nsWebPDecoder::DoDecode(SourceBufferIterator& aIterator, IResumable* aOnResume)
return ReadData();
case SourceBufferIterator::COMPLETE:
return ReadData();
case SourceBufferIterator::WAITING:
return LexerResult(Yield::NEED_MORE_DATA);
default:
MOZ_LOG(sWebPLog, LogLevel::Error,
("[this=%p] nsWebPDecoder::DoDecode -- bad state\n", this));
@ -180,8 +182,16 @@ nsWebPDecoder::CreateFrame(const nsIntRect& aFrameRect)
MOZ_ASSERT(!mDecoder);
MOZ_LOG(sWebPLog, LogLevel::Debug,
("[this=%p] nsWebPDecoder::CreateFrame -- frame %u, %d x %d\n",
this, mCurrentFrame, aFrameRect.width, aFrameRect.height));
("[this=%p] nsWebPDecoder::CreateFrame -- frame %u, (%d, %d) %d x %d\n",
this, mCurrentFrame, aFrameRect.x, aFrameRect.y,
aFrameRect.width, aFrameRect.height));
if (aFrameRect.width <= 0 || aFrameRect.height <= 0) {
MOZ_LOG(sWebPLog, LogLevel::Error,
("[this=%p] nsWebPDecoder::CreateFrame -- bad frame rect\n",
this));
return NS_ERROR_FAILURE;
}
// If this is our first frame in an animation and it doesn't cover the
// full frame, then we are transparent even if there is no alpha
@ -220,6 +230,7 @@ nsWebPDecoder::CreateFrame(const nsIntRect& aFrameRect)
return NS_ERROR_FAILURE;
}
mFrameRect = aFrameRect;
mPipe = std::move(*pipe);
return NS_OK;
}
@ -419,7 +430,9 @@ nsWebPDecoder::ReadSingle(const uint8_t* aData, size_t aLength, const IntRect& a
return LexerResult(Yield::NEED_MORE_DATA);
}
if (width <= 0 || height <= 0 || stride <= 0) {
if (width != mFrameRect.width || height != mFrameRect.height ||
stride < mFrameRect.width * 4 ||
lastRow > mFrameRect.height) {
MOZ_LOG(sWebPLog, LogLevel::Error,
("[this=%p] nsWebPDecoder::ReadSingle -- bad (w,h,s) = (%d, %d, %d)\n",
this, width, height, stride));

5
image/decoders/nsWebPDecoder.h
View file

@ -21,6 +21,8 @@ class nsWebPDecoder final : public Decoder
public:
virtual ~nsWebPDecoder();
DecoderType GetType() const override { return DecoderType::WEBP; }
protected:
LexerResult DoDecode(SourceBufferIterator& aIterator,
IResumable* aOnResume) override;
@ -77,6 +79,9 @@ private:
/// Surface format for the current frame.
gfx::SurfaceFormat mFormat;
/// Frame rect for the current frame.
IntRect mFrameRect;
/// The last row of decoded pixels written to mPipe.
int mLastRow;

24
image/test/gtest/Common.cpp
View file

@ -55,15 +55,7 @@ AutoInitializeImageLib::AutoInitializeImageLib()
// Depending on initialization order, it is possible that our pref changes
// have not taken effect yet because there are pending gfx-related events on
// the main thread.
nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
EXPECT_TRUE(mainThread != nullptr);
bool processed;
do {
processed = false;
nsresult rv = mainThread->ProcessNextEvent(false, &processed);
EXPECT_TRUE(NS_SUCCEEDED(rv));
} while (processed);
SpinPendingEvents();
}
///////////////////////////////////////////////////////////////////////////////
@ -102,6 +94,20 @@ AutoInitializeImageLib::AutoInitializeImageLib()
return rv; \
}
void
SpinPendingEvents()
{
nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
EXPECT_TRUE(mainThread != nullptr);
bool processed;
do {
processed = false;
nsresult rv = mainThread->ProcessNextEvent(false, &processed);
EXPECT_TRUE(NS_SUCCEEDED(rv));
} while (processed);
}
already_AddRefed<nsIInputStream>
LoadFile(const char* aRelativePath)
{

3
image/test/gtest/Common.h
View file

@ -133,6 +133,9 @@ public:
AutoInitializeImageLib();
};
/// Spins on the main thread to process any pending events.
void SpinPendingEvents();
/// Loads a file from the current directory. @return an nsIInputStream for it.
already_AddRefed<nsIInputStream> LoadFile(const char* aRelativePath);

98
image/test/gtest/TestDecoders.cpp
View file

@ -34,6 +34,11 @@ using namespace mozilla::image;
static already_AddRefed<SourceSurface>
CheckDecoderState(const ImageTestCase& aTestCase, Decoder* aDecoder)
{
// Decoder should match what we asked for in the MIME type.
EXPECT_NE(aDecoder->GetType(), DecoderType::UNKNOWN);
EXPECT_EQ(aDecoder->GetType(),
DecoderFactory::GetDecoderType(aTestCase.mMimeType));
EXPECT_TRUE(aDecoder->GetDecodeDone());
EXPECT_EQ(bool(aTestCase.mFlags & TEST_CASE_HAS_ERROR),
aDecoder->HasError());
@ -119,7 +124,8 @@ void WithSingleChunkDecode(const ImageTestCase& aTestCase,
DecoderFlags::FIRST_FRAME_ONLY,
DefaultSurfaceFlags());
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
// Run the full decoder synchronously.
task->Run();
@ -136,6 +142,58 @@ CheckDecoderSingleChunk(const ImageTestCase& aTestCase)
});
}
template <typename Func>
void WithDelayedChunkDecode(const ImageTestCase& aTestCase,
const Maybe<IntSize>& aOutputSize,
Func aResultChecker)
{
nsCOMPtr<nsIInputStream> inputStream = LoadFile(aTestCase.mPath);
ASSERT_TRUE(inputStream != nullptr);
// Figure out how much data we have.
uint64_t length;
nsresult rv = inputStream->Available(&length);
ASSERT_TRUE(NS_SUCCEEDED(rv));
// Prepare an empty SourceBuffer.
auto sourceBuffer = MakeNotNull<RefPtr<SourceBuffer>>();
// Create a decoder.
DecoderType decoderType =
DecoderFactory::GetDecoderType(aTestCase.mMimeType);
RefPtr<Decoder> decoder =
DecoderFactory::CreateAnonymousDecoder(decoderType, sourceBuffer, aOutputSize,
DecoderFlags::FIRST_FRAME_ONLY,
DefaultSurfaceFlags());
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ true);
// Run the full decoder synchronously. It should now be waiting on
// the iterator to yield some data since we haven't written anything yet.
task->Run();
// Writing all of the data should wake up the decoder to complete.
sourceBuffer->ExpectLength(length);
rv = sourceBuffer->AppendFromInputStream(inputStream, length);
ASSERT_TRUE(NS_SUCCEEDED(rv));
sourceBuffer->Complete(NS_OK);
// It would have gotten posted to the main thread to avoid mutex contention.
SpinPendingEvents();
// Call the lambda to verify the expected results.
aResultChecker(decoder);
}
static void
CheckDecoderDelayedChunk(const ImageTestCase& aTestCase)
{
WithDelayedChunkDecode(aTestCase, Nothing(), [&](Decoder* aDecoder) {
CheckDecoderResults(aTestCase, aDecoder);
});
}
static void
CheckDecoderMultiChunk(const ImageTestCase& aTestCase)
{
@ -157,7 +215,8 @@ CheckDecoderMultiChunk(const ImageTestCase& aTestCase)
DecoderFlags::FIRST_FRAME_ONLY,
DefaultSurfaceFlags());
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
for (uint64_t read = 0; read < length ; ++read) {
uint64_t available = 0;
@ -581,6 +640,11 @@ TEST_F(ImageDecoders, PNGSingleChunk)
CheckDecoderSingleChunk(GreenPNGTestCase());
}
TEST_F(ImageDecoders, PNGDelayedChunk)
{
CheckDecoderDelayedChunk(GreenPNGTestCase());
}
TEST_F(ImageDecoders, PNGMultiChunk)
{
CheckDecoderMultiChunk(GreenPNGTestCase());
@ -596,6 +660,11 @@ TEST_F(ImageDecoders, GIFSingleChunk)
CheckDecoderSingleChunk(GreenGIFTestCase());
}
TEST_F(ImageDecoders, GIFDelayedChunk)
{
CheckDecoderDelayedChunk(GreenGIFTestCase());
}
TEST_F(ImageDecoders, GIFMultiChunk)
{
CheckDecoderMultiChunk(GreenGIFTestCase());
@ -611,6 +680,11 @@ TEST_F(ImageDecoders, JPGSingleChunk)
CheckDecoderSingleChunk(GreenJPGTestCase());
}
TEST_F(ImageDecoders, JPGDelayedChunk)
{
CheckDecoderDelayedChunk(GreenJPGTestCase());
}
TEST_F(ImageDecoders, JPGMultiChunk)
{
CheckDecoderMultiChunk(GreenJPGTestCase());
@ -626,6 +700,11 @@ TEST_F(ImageDecoders, BMPSingleChunk)
CheckDecoderSingleChunk(GreenBMPTestCase());
}
TEST_F(ImageDecoders, BMPDelayedChunk)
{
CheckDecoderDelayedChunk(GreenBMPTestCase());
}
TEST_F(ImageDecoders, BMPMultiChunk)
{
CheckDecoderMultiChunk(GreenBMPTestCase());
@ -641,6 +720,11 @@ TEST_F(ImageDecoders, ICOSingleChunk)
CheckDecoderSingleChunk(GreenICOTestCase());
}
TEST_F(ImageDecoders, ICODelayedChunk)
{
CheckDecoderDelayedChunk(GreenICOTestCase());
}
TEST_F(ImageDecoders, ICOMultiChunk)
{
CheckDecoderMultiChunk(GreenICOTestCase());
@ -661,6 +745,11 @@ TEST_F(ImageDecoders, IconSingleChunk)
CheckDecoderSingleChunk(GreenIconTestCase());
}
TEST_F(ImageDecoders, IconDelayedChunk)
{
CheckDecoderDelayedChunk(GreenIconTestCase());
}
TEST_F(ImageDecoders, IconMultiChunk)
{
CheckDecoderMultiChunk(GreenIconTestCase());
@ -676,6 +765,11 @@ TEST_F(ImageDecoders, WebPSingleChunk)
CheckDecoderSingleChunk(GreenWebPTestCase());
}
TEST_F(ImageDecoders, WebPDelayedChunk)
{
CheckDecoderDelayedChunk(GreenWebPTestCase());
}
TEST_F(ImageDecoders, WebPMultiChunk)
{
CheckDecoderMultiChunk(GreenWebPTestCase());

5
image/test/gtest/TestMetadata.cpp
View file

@ -60,7 +60,8 @@ CheckMetadata(const ImageTestCase& aTestCase,
RefPtr<Decoder> decoder =
DecoderFactory::CreateAnonymousMetadataDecoder(decoderType, sourceBuffer);
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
RefPtr<IDecodingTask> task =
new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
if (aBMPWithinICO == BMPWithinICO::YES) {
static_cast<nsBMPDecoder*>(decoder.get())->SetIsWithinICO();
@ -111,7 +112,7 @@ CheckMetadata(const ImageTestCase& aTestCase,
DecoderFlags::FIRST_FRAME_ONLY,
DefaultSurfaceFlags());
ASSERT_TRUE(decoder != nullptr);
task = new AnonymousDecodingTask(WrapNotNull(decoder));
task = new AnonymousDecodingTask(WrapNotNull(decoder), /* aResumable */ false);
if (aBMPWithinICO == BMPWithinICO::YES) {
static_cast<nsBMPDecoder*>(decoder.get())->SetIsWithinICO();