Closed Bug 1285865 Opened 3 years ago Closed 3 years ago

Replace Decoder::WriteInternal() with a Decoder::DoDecode() method that takes a SourceBufferIterator

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(6 files)

Comment 0 in bug 1284031 describes how we need StreamingLexer to drive the process of reading from a SourceBufferIterator so we can implement yielding efficiently and decode animated images a frame at a time.

We'll do this in a few steps so all the changes are individually simple. In this bug, let's refactor things to the point where Decoder::WriteInternal() is replaced with a Decoder::DoDecode() method that takes a SourceBufferIterator as an argument. We'll still let Decode::Decode() drive the whole process (deciding when to advance and when to consider decoding done) for now. We will inline Decoder::Write() into Decoder::Decode(), though, because doing that will make the changes we need to make later a lot cleaner.
This is preparation for inlining Write() into Decode(). Just removing some
checks that Decode() already handles.
Attachment #8769571 - Flags: review?(edwin)
Rather than move the telemetry code as-is into Decode(), let's wrap it in a RAII
class. That'll make things cleaner later as we shuffle code around.
Attachment #8769572 - Flags: review?(edwin)
OK, we're ready to inline Write() into Decode().
Attachment #8769573 - Flags: review?(edwin)
It's kinda silly to have WriteInternal() without Write(), so let's rename
WriteInternal() to DoDecode(), which is more in line with the naming patterns
used in newer ImageLib code anyway. While we're at it, let's replace the
|uint32_t| argument with a |size_t| argument, since this doesn't really require
any code changes and brings the type of the method more in harmony with
StreamingLexer::Lex() and the internals of the decoders.
Attachment #8769575 - Flags: review?(edwin)
Once StreamingLexer is driving the SourceBufferIterator it's going to be the
thing that tells the loop in Decode() what to do. That means that it needs to be
able to communicate with the Decode() loop, which means that DoDecode() should
return the same thing as StreamingLexer::Lex() - a Maybe<TerminalState>. This
also lets use share a small bit of error-handling code that all the decoders had
in common.
Attachment #8769577 - Flags: review?(edwin)
Finally, we're ready to pass a SourceBufferIterator to DoDecode(). Note that
DoDecode() and StreamingLexer just read the current data available from the
SourceBufferIterator that gets passed to them. We're not ready for them to be
responsible for advancing the iterator yet, but we'll get there.
Attachment #8769578 - Flags: review?(edwin)
Blocks: 1285867
Comment on attachment 8769573 [details] [diff] [review]
(Part 3) - Inline Decoder::Write() into Decoder::Decode().

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

::: image/Decoder.cpp
@@ +146,5 @@
>      MOZ_ASSERT(newState == SourceBufferIterator::READY);
>  
> +    {
> +      PROFILER_LABEL("ImageDecoder", "Write",
> +        js::ProfileEntry::Category::GRAPHICS);

Need to update the profiler label.
Attachment #8769573 - Flags: review?(edwin) → review+
Comment on attachment 8769575 [details] [diff] [review]
(Part 4) - Rename Decoder::WriteInternal() to Decoder::DoDecode() and fix its argument types.

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

::: image/decoders/nsBMPDecoder.h
@@ +148,5 @@
>      mMayHaveTransparency = true;
>      mDoesHaveTransparency = true;
>    }
>  
> +  void DoDecode(const char* aBuffer, size_t aLength) override;

I'm not a fan of dropping the |virtual| keyword even though it's implicit. Mostly for aesthetic reasons (and maybe clarity?) however, so I won't insist it be changed.
Attachment #8769575 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> Need to update the profiler label.

Never mind; I see you've done this elsewhere.
Thanks for the reviews, Edwin!

(In reply to Edwin Flores [:eflores] [:edwin] from comment #9)
> I'm not a fan of dropping the |virtual| keyword even though it's implicit.
> Mostly for aesthetic reasons (and maybe clarity?) however, so I won't insist
> it be changed.

I'm in your camp on this but alas our C++ style guide says we're only supposed to use one of |virtual| or |override|.
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3900aecb4153
(Part 1) - Remove Decoder::Write() checks that are redundant with Decoder::Decode(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d98e5a17a183
(Part 2) - Add a RAII class to record decoder telemetry. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6188761ca7f4
(Part 3) - Inline Decoder::Write() into Decoder::Decode(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6735ebca3084
(Part 4) - Rename Decoder::WriteInternal() to Decoder::DoDecode() and fix its argument types. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c717a8dc382
(Part 5) - Return a Maybe<TerminalState> from Decoder::DoDecode(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/940b6ad95cfc
(Part 6) - Pass a SourceBufferIterator to Decoder::DoDecode(). r=edwin
You need to log in before you can comment on or make changes to this bug.