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)
Core
Graphics: ImageLib
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
|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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
Here's a try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0443161abe52
Attachment #8769585 -
Flags: review?(edwin) → review+
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+
Attachment #8769592 -
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+
Attachment #8769590 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
Ah, I see I already filed that followup. It's bug 1286761.
Blocks: 1286761
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
Comment 28•8 years ago
|
||
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
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77fe4e663e34 https://hg.mozilla.org/mozilla-central/rev/8c5e700ca1ae https://hg.mozilla.org/mozilla-central/rev/560ca5d7336e https://hg.mozilla.org/mozilla-central/rev/c6a434794e0e https://hg.mozilla.org/mozilla-central/rev/85899fc1c974 https://hg.mozilla.org/mozilla-central/rev/42d999dd75fc https://hg.mozilla.org/mozilla-central/rev/5621495f2102 https://hg.mozilla.org/mozilla-central/rev/d22b7fe7aceb https://hg.mozilla.org/mozilla-central/rev/3ddd65b25581 https://hg.mozilla.org/mozilla-central/rev/4840df0f7046 https://hg.mozilla.org/mozilla-central/rev/9b650c8855c2 https://hg.mozilla.org/mozilla-central/rev/401978e000d8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•