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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Clean up existing image flags

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add FLAG_SYNC_DECODE_IF_FAST

This part adds FLAG_SYNC_DECODE_IF_FAST and uses it to reimplement the behavior
of StartDecoding().

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add FLAG_ASYNC_NOTIFY

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Remove DecodeStrategy and WantDecodedFrames

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=6ad6e1c35398

Comment 7

[Timothy Nikkel (:tnikkel)]

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 8

[Timothy Nikkel (:tnikkel)]

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 :)

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! I'll make those changes.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add FLAG_SYNC_DECODE_IF_FAST

Addressed review comments.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add FLAG_ASYNC_NOTIFY

Addressed review comments.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3bc48fee74
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef2293e754d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f767dbc3f0c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/595be1d3de40

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec3bc48fee74
https://hg.mozilla.org/mozilla-central/rev/9ef2293e754d
https://hg.mozilla.org/mozilla-central/rev/f767dbc3f0c1
https://hg.mozilla.org/mozilla-central/rev/595be1d3de40

Comment 14

[Seth Fowler [:seth] [:s2h]]

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.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Pushed to Aurora:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/06fb0d5fe0d7
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/eba6fc85e742
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4626a2f29ca5
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/91694fca2ff8

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

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.