Closed Bug 1285867 Opened 8 years ago Closed 8 years ago

Eliminate dependencies on Decoder's internal state in Decoder::Decode()'s main loop

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 2 open bugs)

Details

Attachments

(12 files, 7 obsolete files)

20.61 KB, patch
Details | Diff | Splinter Review
19.36 KB, patch
Details | Diff | Splinter Review
1.06 KB, patch
Details | Diff | Splinter Review
3.54 KB, patch
Details | Diff | Splinter Review
1.50 KB, patch
Details | Diff | Splinter Review
3.18 KB, patch
Details | Diff | Splinter Review
4.59 KB, patch
Details | Diff | Splinter Review
1.88 KB, patch
Details | Diff | Splinter Review
7.53 KB, patch
Details | Diff | Splinter Review
3.59 KB, patch
Details | Diff | Splinter Review
10.79 KB, patch
Details | Diff | Splinter Review
6.47 KB, patch
Details | Diff | Splinter Review
We want to move the main loop of Decoder::Decode() into StreamingLexer in bug 1284031, but it's difficult to do that right now because the loop depends upon internal Decoder state that we don't want to move into StreamingLexer.

In this bug, we'll refactor things so that we only manipulate that state outside of the loop, and the loop itself is totally dependent on the Maybe<TerminalState> value that StreamingLexer::Lex() returns. This is really all we need. If we're done, we get a Some() value, and whether we decoded successfully or failed due to an error is determined by whether we have a Some(TerminalState::SUCCESS) value or a Some(TerminalState::FAILURE) value. There's really nothing else that the loop needs to know to do its job.
Let's simplify the task by removing the notion of "decoder errors" totally. The
distinction between decoder errors and data errors isn't really something we can
usefully make while the decoder is running. (And indeed, we don't right now;
virtually all code that interacts with decoders just calls Decoder::HasError().)
It *is* useful to have a way of returning failures during decoder
initialization, though, so we replace the decoder error concept with a simple
|nsresult| return value from Decoder::Init().
Attachment #8769585 - Flags: review?(edwin)
Data errors, the other kind of error decoders can generate, *are* still useful.
However, we don't really need the Decoder::PostDataError() method to generate
them. Since all decoders use StreamingLexer now, we're *already* returning
TerminalState::FAILURE in virtually all cases where we call PostDataError(). So
let's move totally over to using that, and eliminate all calls to
PostDataError() from Decoder subclasses.

This is important for the overall goal of this bug not only because it makes
things simpler, but also because we don't want out-of-band information to
control whether the main loop in Decode() keeps going or stops, and
PostDataError() is one source of that kind of out-of-band information.
Attachment #8769588 - Flags: review?(edwin)
This is the meatiest patch in this series. What we want to do here is only call
PostDataError() *outside* the main loop in Decode(). To enable that, the loop's
error handling behavior must be controlled by the Maybe<TerminalState> value
that gets returned from DoDecode(). That involves some restructuring of the loop
to, again, reduce reliance on out-of-band information. (I probably could've
split this into a couple of different patches, in retrospect, because I ended up
moving this code over to using a |switch| statement instead of |if| statements,
which may have made it a little harder to review; hopefully it's not too bad.)

The rest of the patches are a lot simpler than this one. We're almost there!
Attachment #8769590 - Flags: review?(edwin)
This patch finally commits us to controlling the loop *only* by checking if
we've reached a terminal state. I added an assertion to check that nothing went
wrong in this transformation.
Attachment #8769591 - Flags: review?(edwin)
|mDataDone| is not really the question anymore; we don't use it to control the
loop, and indeed we don't care about it as long as a terminal state has been
reached. (It's possible for an image to be followed by garbage data, and that's
fine as long as we can successfully decode the image itself. This is obviously
an edge case, though.) It makes more sense to explicitly track whether we've
reached a terminal state.
Attachment #8769592 - Flags: review?(edwin)
There are a couple of kinds of telemetry that we record for decoders.

One kind is timing-based. There's really not much we can do here; we need to
move the timing telemetry outside the loop. The main consequence of that is that
SourceBuffer::AdvanceOrScheduleResume() ends up being included in the timing
data now. In cases where there is contention between readers and the writer on
the SourceBuffer, this may cause the telemetry numbers to look a little worse.
It's useful for us to know if something regresses in that area, though, so I'd
argue it's a beneficial change.

The other kind of telemetry just tracks how many bytes the decoder has processed
and how many chunks it received those bytes in. Essentially each time
AdvanceOrScheduleResume() returns data, that's a chunk. An image which has fully
loaded from the network will normally be delivered in one large chunk, but there
can be more if the image is loading slowly. What's really unfortunate is that
this is not *quite* the same as the notion of a chunk used internally in
SourceBuffer; this is much older terminology than SourceBuffer and I need to
clean it up. (I'll file a followup bug to do it.) At any rate, we can just track
this information on the SourceBufferIterator itself without much trouble. In
fact, this is better than the current system, because we'll find that very
useful when implementing a GTest suite for SourceBuffer in an upcoming bug.
Attachment #8769594 - Flags: review?(edwin)
OK, at this point the goal is completed and Decode()'s main loop doesn't rely on
any Decoder state. It's worth doing a little cleanup here, though. There are
still some references to "data errors" in the code, but since we eliminated
"decoder errors", there's really only one kind of error state a decoder can
have. Let's change those references to just say "errors", full stop.
Attachment #8769595 - Flags: review?(edwin)
Blocks: 1285873
Comment on attachment 8769588 [details] [diff] [review]
(Part 2) - Don't call Decoder::PostDataError() from Decoder subclasses.

Review of attachment 8769588 [details] [diff] [review]:
-----------------------------------------------------------------

Probably worth making PostDataError() private to enforce this.

::: image/decoders/nsICODecoder.h
@@ +71,5 @@
>    size_t FirstResourceOffset() const;
>  
>    Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) override;
> +  nsresult FinishInternal() override;
> +  nsresult FinishWithErrorInternal() override;

As I said in bug 1285865, EIBTI, but not overly fussed.
Attachment #8769588 - Flags: review?(edwin) → review+
Comment on attachment 8769591 [details] [diff] [review]
(Part 4) - Decide whether we're done decoding by checking if we've reached a terminal state.

Review of attachment 8769591 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/Decoder.cpp
@@ +132,5 @@
> +      MOZ_ASSERT_UNREACHABLE("Finished decode without reaching terminal state?");
> +      terminalState = Some(TerminalState::SUCCESS);
> +      break;
> +    }
> +  

nit: whitespace
Attachment #8769591 - Flags: review?(edwin) → review+
Comment on attachment 8769594 [details] [diff] [review]
(Part 6) - Record Decoder telemetry outside of the loop.

Review of attachment 8769594 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/Decoder.cpp
@@ +133,3 @@
>          break;
>        }
> +    

nit: whitespace
Attachment #8769594 - Flags: review?(edwin) → review+
Comment on attachment 8769595 [details] [diff] [review]
(Part 7) - Clean up remaining references to decoder 'data errors' and refer to them as just 'errors'.

Review of attachment 8769595 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/Decoder.h
@@ +365,5 @@
>    // means a single iteration, stopping on the last frame.
>    void PostDecodeDone(int32_t aLoopCount = 0);
>  
> +  // Report that an error was encountered while decoding.
> +  void PostError();

Probably worth having this private, as in comment 9.
Attachment #8769595 - Flags: review?(edwin) → review+
Blocks: 1263756
Blocks: 1286165
OK, I've fixed all the issues that appeared on try. The main problems were:

- The BMP decoder fired PostSize() before PostHasTransparency(), and it fired them in two different lexer states. If the input data was delivered in an unlucky way so that we hit the boundary perfectly, we could fire PostSize() and exit the loop (because GetDecodeDone() checks if |IsMetadataDecode() && HasSize()|) without PostHasTransparency() being called, which obviously caused opacity issues.

- There was a nasty interaction between the BMP decoder and the ICO decoder. Based the BMP might be opaque on its own, but the AND mask we decode at the ICO level could give it transparency. nsBMPDecoder::FinishInternal() fires PostFrameStop(), which among other things allows the decoder to tell imgFrame "Hey, I know we created an RGBA frame, but I didn't actually see any transparent pixels so *really* this frame should be RGBX." So what's the problem? Prior to these patches, the two decoders interacted like this:

ICODecoder: Start decoding.
BMPDecoder: Start decoding.
ICO Decoder: Detect that we've decoded the entire BMP and decode the AND mask.
BMPDecoder: nsBMPDecoder::FinishInternal() gets called.
ICO Decoder: nsICODecoder::FinishInternal() gets called.

This was caused by a bug in the loop conditions in Decoder::Decode(). Because of that incorrect nesting, nsBMPDecoder::FinishInternal() would correctly report transparency if it was introduced by the AND mask. However, these patches changed the nesting to this:

ICODecoder: Start decoding.
BMPDecoder: Start decoding.
BMPDecoder: nsBMPDecoder::FinishInternal() gets called.
ICO Decoder: Detect that we've decoded the entire BMP and decode the AND mask.
ICO Decoder: nsICODecoder::FinishInternal() gets called.

This is the correct and sane nesting, but it means that at the time that nsBMPDecoder::FinishInternal() gets called, it doesn't know about the transparency introduced by the AND mask, and we get the wrong opacity value, which again causes incorrect rendering.

I've made a one line fix in this patch (well, one line plus comments) which just always treats BMP-in-ICO as transparent. I'm going to file a followup to move the AND mask decoding into nsBMPDecoder, where it belongs, which would solve this problem in a *much* more sane manner.
Ah, I see I already filed that followup. It's bug 1286761.
Blocks: 1286761
I'm going to reupload the patches. The only changes are:

- Tiny fixes for the issues mentioned above.
- I threw some new tests in.
- I split part 3 into a bunch of separate pieces that each make a tiny change. They all compile separately and pass tests separately. This is just to make any regression hunting we need to do easier.
Attachment #8769585 - Attachment is obsolete: true
Attachment #8769588 - Attachment is obsolete: true
Attachment #8769590 - Attachment is obsolete: true
Attachment #8769591 - Attachment is obsolete: true
Attachment #8769592 - Attachment is obsolete: true
Attachment #8769594 - Attachment is obsolete: true
Attachment #8769595 - Attachment is obsolete: true
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77fe4e663e34
(Part 1) - Remove Decoder's notion of decoder errors. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5e700ca1ae
(Part 2) - Don't call Decoder::PostDataError() from Decoder subclasses. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/560ca5d7336e
(Part 3a) - Don't attempt to keep decoding if we're already done. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a434794e0e
(Part 3b) - Replace the series of |if| statements in the Decode() loop with a |switch|. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/85899fc1c974
(Part 3c) - Replace the Decode() |while| loop with a |do| loop. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/42d999dd75fc
(Part 3d) - Rely on TerminalState to decide when to post errors inside the loop. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5621495f2102
(Part 3e) - Use TerminalState to exit the Decode() loop. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22b7fe7aceb
(Part 3f) - Only call PostDataError() outside the loop. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddd65b25581
(Part 4) - Decide whether we're done decoding by checking if we've reached a terminal state. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/4840df0f7046
(Part 5) - Replace Decoder::mDataDone with Decoder::mReachedTerminalState. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b650c8855c2
(Part 6) - Record Decoder telemetry outside of the loop. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/401978e000d8
(Part 7) - Clean up remaining references to decoder 'data errors' and refer to them as just 'errors'. r=edwin
You need to log in before you can comment on or make changes to this bug.