Skip to content

Commit

Permalink
Bug 1813466 - Store AVIF prefs as default decoder flags in RasterImag…
Browse files Browse the repository at this point in the history 
…e. r=tnikkel

This prevents a crash that would occur if a redecode of an animated AVIF happened after changing the `sequences.enabled` pref from `true` to `false`.

Differential Revision: https://phabricator.services.mozilla.com/D170190
  • Loading branch information
@Zaggy1024
Zaggy1024 committed Mar 17, 2023
1 parent b2f1baf commit ef5aaf4
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 33 deletions.
21 changes: 20 additions & 1 deletion image/DecoderFactory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,24 @@ DecoderType DecoderFactory::GetDecoderType(const char* aMimeType) {
return type;
}

/* static */
DecoderFlags DecoderFactory::GetDefaultDecoderFlagsForType(DecoderType aType) {
auto flags = DefaultDecoderFlags();

#ifdef MOZ_AV1
if (aType == DecoderType::AVIF) {
if (StaticPrefs::image_avif_sequence_enabled()) {
flags |= DecoderFlags::AVIF_SEQUENCES_ENABLED;
}
if (StaticPrefs::image_avif_sequence_animate_avif_major_branded_images()) {
flags |= DecoderFlags::AVIF_ANIMATE_AVIF_MAJOR;
}
}
#endif

return flags;
}

/* static */
already_AddRefed<Decoder> DecoderFactory::GetDecoder(DecoderType aType,
RasterImage* aImage,
Expand Down Expand Up @@ -294,7 +312,7 @@ already_AddRefed<Decoder> DecoderFactory::CloneAnimationDecoder(

/* static */
already_AddRefed<IDecodingTask> DecoderFactory::CreateMetadataDecoder(
DecoderType aType, NotNull<RasterImage*> aImage,
DecoderType aType, NotNull<RasterImage*> aImage, DecoderFlags aFlags,
NotNull<SourceBuffer*> aSourceBuffer) {
if (aType == DecoderType::UNKNOWN) {
return nullptr;
Expand All @@ -306,6 +324,7 @@ already_AddRefed<IDecodingTask> DecoderFactory::CreateMetadataDecoder(

// Initialize the decoder.
decoder->SetMetadataDecode(true);
decoder->SetDecoderFlags(aFlags);
decoder->SetIterator(aSourceBuffer->Iterator());

if (NS_FAILED(decoder->Init())) {
Expand Down
5 changes: 4 additions & 1 deletion image/DecoderFactory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ class DecoderFactory {
/// @return the type of decoder which is appropriate for @aMimeType.
static DecoderType GetDecoderType(const char* aMimeType);

/// @return the default flags to use when creating a decoder of @aType.
static DecoderFlags GetDefaultDecoderFlagsForType(DecoderType aType);

/**
* Creates and initializes a decoder for non-animated images of type @aType.
* (If the image *is* animated, only the first frame will be decoded.) The
Expand Down Expand Up @@ -128,7 +131,7 @@ class DecoderFactory {
* from.
*/
static already_AddRefed<IDecodingTask> CreateMetadataDecoder(
DecoderType aType, NotNull<RasterImage*> aImage,
DecoderType aType, NotNull<RasterImage*> aImage, DecoderFlags aFlags,
NotNull<SourceBuffer*> aSourceBuffer);

/**
Expand Down
11 changes: 11 additions & 0 deletions image/DecoderFlags.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@ enum class DecoderFlags : uint8_t {
* set.
*/
CANNOT_SUBSTITUTE = 1 << 4,

#ifdef MOZ_AV1
// The flags below are stored in RasterImage to allow a decoded image to
// remain consistent in whether it is animated or not.

// Set according to the "image.avif.sequence.enabled" preference.
AVIF_SEQUENCES_ENABLED = 1 << 5,
// Set according to the
// "image.avif.sequence.animate_avif_major_branded_images" preference.
AVIF_ANIMATE_AVIF_MAJOR = 1 << 6,
#endif
};
MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(DecoderFlags)

Expand Down
9 changes: 7 additions & 2 deletions image/RasterImage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ nsresult RasterImage::Init(const char* aMimeType, uint32_t aFlags) {
SurfaceCache::LockImage(ImageKey(this));
}

// Set the default flags according to the decoder type to allow preferences to
// be stored if necessary.
mDefaultDecoderFlags =
DecoderFactory::GetDefaultDecoderFlagsForType(mDecoderType);

// Mark us as initialized
mInitialized = true;

Expand Down Expand Up @@ -1173,7 +1178,7 @@ void RasterImage::Decode(const OrientedIntSize& aSize, uint32_t aFlags,
SurfaceCache::UnlockEntries(ImageKey(this));

// Determine which flags we need to decode this image with.
DecoderFlags decoderFlags = DefaultDecoderFlags();
DecoderFlags decoderFlags = mDefaultDecoderFlags;
if (aFlags & FLAG_ASYNC_NOTIFY) {
decoderFlags |= DecoderFlags::ASYNC_NOTIFY;
}
Expand Down Expand Up @@ -1257,7 +1262,7 @@ RasterImage::DecodeMetadata(uint32_t aFlags) {

// Create a decoder.
RefPtr<IDecodingTask> task = DecoderFactory::CreateMetadataDecoder(
mDecoderType, WrapNotNull(this), mSourceBuffer);
mDecoderType, WrapNotNull(this), mDefaultDecoderFlags, mSourceBuffer);

// Make sure DecoderFactory was able to create a decoder successfully.
if (!task) {
Expand Down
6 changes: 6 additions & 0 deletions image/RasterImage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,12 @@ class RasterImage final : public ImageResource,

TimeStamp mDrawStartTime;

// This field is set according to the DecoderType of this image once when
// initialized so that a decoder's flags can be set according to any
// preferences that affect its behavior in a way that would otherwise cause
// errors, such as enabling or disabling animation.
DecoderFlags mDefaultDecoderFlags = DefaultDecoderFlags();

//////////////////////////////////////////////////////////////////////////////
// Scaling.
//////////////////////////////////////////////////////////////////////////////
Expand Down
46 changes: 25 additions & 21 deletions image/decoders/nsAVIFDecoder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,14 @@ AVIFParser::~AVIFParser() {
}

Mp4parseStatus AVIFParser::Create(const Mp4parseIo* aIo, ByteStream* aBuffer,
UniquePtr<AVIFParser>& aParserOut) {
UniquePtr<AVIFParser>& aParserOut,
bool aAllowSequences,
bool aAnimateAVIFMajor) {
MOZ_ASSERT(aIo);
MOZ_ASSERT(!aParserOut);

UniquePtr<AVIFParser> p(new AVIFParser(aIo));
Mp4parseStatus status = p->Init(aBuffer);
Mp4parseStatus status = p->Init(aBuffer, aAllowSequences, aAnimateAVIFMajor);

if (status == MP4PARSE_STATUS_OK) {
MOZ_ASSERT(p->mParser);
Expand Down Expand Up @@ -308,7 +310,8 @@ static Mp4parseStatus CreateSampleIterator(
return MP4PARSE_STATUS_OK;
}

Mp4parseStatus AVIFParser::Init(ByteStream* aBuffer) {
Mp4parseStatus AVIFParser::Init(ByteStream* aBuffer, bool aAllowSequences,
bool aAnimateAVIFMajor) {
#define CHECK_MP4PARSE_STATUS(v) \
do { \
if ((v) != MP4PARSE_STATUS_OK) { \
Expand All @@ -333,26 +336,21 @@ Mp4parseStatus AVIFParser::Init(ByteStream* aBuffer) {
status = mp4parse_avif_get_info(mParser.get(), &mInfo);
CHECK_MP4PARSE_STATUS(status);

bool shouldAnimate = mInfo.has_sequence;

// Disable animation if the specified major brand is avif or avis is preffed
// off.
if (shouldAnimate) {
if (!StaticPrefs::image_avif_sequence_enabled()) {
shouldAnimate = false;
bool useSequence = mInfo.has_sequence;
if (useSequence) {
if (!aAllowSequences) {
MOZ_LOG(sAVIFLog, LogLevel::Debug,
("[this=%p] AVIF sequences disabled", this));
} else if (shouldAnimate &&
!StaticPrefs::
image_avif_sequence_animate_avif_major_branded_images() &&
!memcmp(mInfo.major_brand, "avif", sizeof(mInfo.major_brand))) {
shouldAnimate = false;
useSequence = false;
} else if (!aAnimateAVIFMajor &&
!!memcmp(mInfo.major_brand, "avis", sizeof(mInfo.major_brand))) {
useSequence = false;
MOZ_LOG(sAVIFLog, LogLevel::Debug,
("[this=%p] AVIF prefers still image", this));
}
}

if (shouldAnimate) {
if (useSequence) {
status = CreateSampleIterator(parser, aBuffer, mInfo.color_track_id,
mColorSampleIter);
CHECK_MP4PARSE_STATUS(status);
Expand Down Expand Up @@ -1202,11 +1200,14 @@ LexerResult nsAVIFDecoder::DoDecode(SourceBufferIterator& aIterator,
return LexerResult(Yield::NEED_MORE_DATA);
}
if (r == NonDecoderResult::OutputAvailable) {
MOZ_ASSERT(HasSize());
return LexerResult(Yield::OUTPUT_AVAILABLE);
}
return r == NonDecoderResult::Complete
? LexerResult(TerminalState::SUCCESS)
: LexerResult(TerminalState::FAILURE);
if (r == NonDecoderResult::Complete) {
MOZ_ASSERT(HasSize());
return LexerResult(TerminalState::SUCCESS);
}
return LexerResult(TerminalState::FAILURE);
}

MOZ_ASSERT(result.is<Dav1dResult>() || result.is<AOMResult>() ||
Expand All @@ -1226,8 +1227,11 @@ Mp4parseStatus nsAVIFDecoder::CreateParser() {
if (!mParser) {
Mp4parseIo io = {nsAVIFDecoder::ReadSource, this};
mBufferStream = new AVIFDecoderStream(&mBufferedData);
Mp4parseStatus status =
AVIFParser::Create(&io, mBufferStream.get(), mParser);

Mp4parseStatus status = AVIFParser::Create(
&io, mBufferStream.get(), mParser,
bool(GetDecoderFlags() & DecoderFlags::AVIF_SEQUENCES_ENABLED),
bool(GetDecoderFlags() & DecoderFlags::AVIF_ANIMATE_AVIF_MAJOR));

if (status != MP4PARSE_STATUS_OK) {
return status;
Expand Down
6 changes: 4 additions & 2 deletions image/decoders/nsAVIFDecoder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ struct AVIFImage {
class AVIFParser {
public:
static Mp4parseStatus Create(const Mp4parseIo* aIo, ByteStream* aBuffer,
UniquePtr<AVIFParser>& aParserOut);
UniquePtr<AVIFParser>& aParserOut,
bool aAllowSequences, bool aAnimateAVIFMajor);

~AVIFParser();

Expand All @@ -131,7 +132,8 @@ class AVIFParser {
private:
explicit AVIFParser(const Mp4parseIo* aIo);

Mp4parseStatus Init(ByteStream* aBuffer);
Mp4parseStatus Init(ByteStream* aBuffer, bool aAllowSequences,
bool aAnimateAVIFMajor);

struct FreeAvifParser {
void operator()(Mp4parseAvifParser* aPtr) { mp4parse_avif_free(aPtr); }
Expand Down
15 changes: 11 additions & 4 deletions image/test/gtest/TestDecoders.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,11 @@ void WithSingleChunkDecode(const ImageTestCase& aTestCase,

// Create a decoder.
DecoderType decoderType = DecoderFactory::GetDecoderType(aTestCase.mMimeType);
DecoderFlags decoderFlags =
DecoderFactory::GetDefaultDecoderFlagsForType(decoderType) |
DecoderFlags::FIRST_FRAME_ONLY;
RefPtr<image::Decoder> decoder = DecoderFactory::CreateAnonymousDecoder(
decoderType, sourceBuffer, aOutputSize, DecoderFlags::FIRST_FRAME_ONLY,
decoderType, sourceBuffer, aOutputSize, decoderFlags,
aTestCase.mSurfaceFlags);
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task =
Expand Down Expand Up @@ -233,8 +236,11 @@ static void CheckDecoderMultiChunk(const ImageTestCase& aTestCase,
auto sourceBuffer = MakeNotNull<RefPtr<SourceBuffer>>();
sourceBuffer->ExpectLength(length);
DecoderType decoderType = DecoderFactory::GetDecoderType(aTestCase.mMimeType);
DecoderFlags decoderFlags =
DecoderFactory::GetDefaultDecoderFlagsForType(decoderType) |
DecoderFlags::FIRST_FRAME_ONLY;
RefPtr<image::Decoder> decoder = DecoderFactory::CreateAnonymousDecoder(
decoderType, sourceBuffer, Nothing(), DecoderFlags::FIRST_FRAME_ONLY,
decoderType, sourceBuffer, Nothing(), decoderFlags,
aTestCase.mSurfaceFlags);
ASSERT_TRUE(decoder != nullptr);
RefPtr<IDecodingTask> task =
Expand Down Expand Up @@ -381,15 +387,16 @@ static void WithSingleChunkAnimationDecode(const ImageTestCase& aTestCase,
// Create a metadata decoder first, because otherwise RasterImage will get
// unhappy about finding out the image is animated during a full decode.
DecoderType decoderType = DecoderFactory::GetDecoderType(aTestCase.mMimeType);
DecoderFlags decoderFlags =
DecoderFactory::GetDefaultDecoderFlagsForType(decoderType);
RefPtr<IDecodingTask> task = DecoderFactory::CreateMetadataDecoder(
decoderType, rasterImage, sourceBuffer);
decoderType, rasterImage, decoderFlags, sourceBuffer);
ASSERT_TRUE(task != nullptr);

// Run the metadata decoder synchronously.
task->Run();

// Create a decoder.
DecoderFlags decoderFlags = DefaultDecoderFlags();
SurfaceFlags surfaceFlags = aTestCase.mSurfaceFlags;
RefPtr<image::Decoder> decoder = DecoderFactory::CreateAnonymousDecoder(
decoderType, sourceBuffer, Nothing(), decoderFlags, surfaceFlags);
Expand Down
5 changes: 3 additions & 2 deletions image/test/gtest/TestFrameAnimator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,10 @@ static void WithFrameAnimatorDecode(const ImageTestCase& aTestCase,
// Create a metadata decoder first, because otherwise RasterImage will get
// unhappy about finding out the image is animated during a full decode.
DecoderType decoderType = DecoderFactory::GetDecoderType(aTestCase.mMimeType);
DecoderFlags decoderFlags =
DecoderFactory::GetDefaultDecoderFlagsForType(decoderType);
RefPtr<IDecodingTask> task = DecoderFactory::CreateMetadataDecoder(
decoderType, rasterImage, sourceBuffer);
decoderType, rasterImage, decoderFlags, sourceBuffer);
ASSERT_TRUE(task != nullptr);

// Run the metadata decoder synchronously.
Expand All @@ -85,7 +87,6 @@ static void WithFrameAnimatorDecode(const ImageTestCase& aTestCase,

// Create an AnimationSurfaceProvider which will manage the decoding process
// and make this decoder's output available in the surface cache.
DecoderFlags decoderFlags = DefaultDecoderFlags();
SurfaceFlags surfaceFlags = aTestCase.mSurfaceFlags;
rv = DecoderFactory::CreateAnimationDecoder(
decoderType, rasterImage, sourceBuffer, aTestCase.mSize, decoderFlags,
Expand Down

0 comments on commit ef5aaf4

Please sign in to comment.