Closed Bug 1128223 Opened 5 years ago Closed 5 years ago

Add new flags to imgIContainer to give callers more control over what is done synchronously

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(4 files, 2 obsolete files)

Right now there are some decisions about what is performed synchronously that aren't specified explicitly by imgIContainer flags:

- Should we try to synchronously decode the image if it can be done quickly? Callers can request this by calling StartDecoding() instead of RequestDecode(), but there are many imgIContainer methods that can trigger decoding, and we don't want two variants of all of them.

- Should we send notifications that result from decoding synchronously or asynchronously? Right now the policy around this isn't really explicitly specified in the API, but it should be, because this is often important to callers.

In this bug we'll add two new imgIContainer flags, FLAG_SYNC_DECODE_IF_FAST and FLAG_ASYNC_NOTIFY, to give callers more control over these behaviors.
Blocks: 1125055
This first part contains initial refactoring that cleans things up a bit and
makes the remaining parts easier to implement. The non-cosmetic things that are
changed are:

- The flags are now unsigned values, which is how they're actually passed around
  anyway. This makes more semantic sense, and it avoids some compiler warnings
  in later patches.

- DECODE_FLAGS_DEFAULT is moved into imgIContainer.idl instead of being a C
  macro in RasterImage.cpp.

- DECODE_FLAGS_MASK is gone; anything that used it now calls the DecodeFlags()
  constexpr function, which is moved into RasterImage.h.

- A Decoder now gets passed all the flags, not only the flags that affect the
  output of the decoder. This requires a few changes in various places, but it
  means that we can pass the decoder flags that affect its *behavior* without
  changing its *output*, which will be useful later.
Attachment #8557549 - Flags: review?(tnikkel)
Ah, I forgot to mention it, but part 1 also removes the duplication of the decoding-output-related flags in Decoder.h, which is a bug waiting to happen. I wanted to remove that before starting to rearrange the flags in later parts.
This part adds FLAG_SYNC_DECODE_IF_FAST and uses it to reimplement the behavior
of StartDecoding().
Attachment #8557550 - Flags: review?(tnikkel)
Attached patch (Part 3) - Add FLAG_ASYNC_NOTIFY (obsolete) — Splinter Review
This part adds FLAG_ASYNC_NOTIFY. GetCurrentImage() is switched to use it. We
don't need the aShouldSyncNotify parameter to LookupFrame anymore as a result.
Attachment #8557551 - Flags: review?(tnikkel)
The changes in the previous parts end up meaning that we no longer need either
the DecodeStrategy enumeration (which is replaced by the existing and new
flags), or WantDecodedFrames (which is now so simple that it doesn't add any
value). In this part both of them get removed.
Attachment #8557552 - Flags: review?(tnikkel)
Blocks: 1128225
Attachment #8557549 - Flags: review?(tnikkel) → review+
Attachment #8557550 - Flags: review?(tnikkel) → review+
Comment on attachment 8557550 [details] [diff] [review]
(Part 2) - Add FLAG_SYNC_DECODE_IF_FAST

>+   * FLAG_SYNC_DECODE_IF_FAST: Like FLAG_SYNC_DECODE, but requests a sync decode
>+   * be performed only if ImageLib estimates it can be completely very quickly.
>+   *

Completed
Comment on attachment 8557551 [details] [diff] [review]
(Part 3) - Add FLAG_ASYNC_NOTIFY

>+   * FLAG_ASYNC_NOTIFY: Send notifications asynchronously, even if we decode
>+   * synchronously beause of FLAG_SYNC_DECODE or FLAG_SYNC_DECODE.

One of the FLAG_SYNC_DECODE should be changed to what you actually meant :)
Attachment #8557551 - Flags: review?(tnikkel) → review+
Attachment #8557552 - Flags: review?(tnikkel) → review+
Thanks for the review! I'll make those changes.
Addressed review comments.
Attachment #8557550 - Attachment is obsolete: true
Addressed review comments.
Attachment #8557551 - Attachment is obsolete: true
Depends on: 1129818
Comment on attachment 8557549 [details] [diff] [review]
(Part 1) - Clean up existing image flags

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[User impact if declined]: Already approved.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time.
[String/UUID change made/needed]: None.
Attachment #8557549 - Flags: approval-mozilla-aurora?
Attachment #8557552 - Flags: approval-mozilla-aurora?
Attachment #8558322 - Flags: approval-mozilla-aurora?
Attachment #8558326 - Flags: approval-mozilla-aurora?
Comment on attachment 8557549 [details] [diff] [review]
(Part 1) - Clean up existing image flags

This is part of a collection of ImageLib fixes that have been tested by QE and that Seth had approval to land. Adding approval to the bug after the fact.
Attachment #8557549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8557552 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8558322 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8558326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.