SkStream::isAtEnd semantics

[Nigel Tao]

Here's the SkStream::isAtEnd doc comment
(https://skia.googlesource.com/skia/+/bc8ca57868d22e31481f05d7ad836ddf6a3a4cf7/include/core/SkStream.h#87)

/** Returns true when all the bytes in the stream have been read.
* This may return true early (when there are no more bytes to be read)
* or late (after the first unsuccessful read).
*/
virtual bool isAtEnd() const = 0;

If reading a traditional file or a SkData, whose size is known at
construction (ignoring the fact that files' sizes are mutable), then
the "all the bytes in the stream have been read" semantics are
straightforward.

But suppose that I'm reading from a pipe or a network socket. If I've
read 100 bytes and 100 bytes were available *so far*, but I don't know
whether more bytes are coming, should isAtEnd return true or false?
Does "all the bytes in the stream" mean "all the bytes, at this point
in time" or "all the bytes, now and in the future"?

In Skia's repository, src/codec/SkWuffsCodec.cpp says
(https://skia.googlesource.com/skia/+/bc8ca57868d22e31481f05d7ad836ddf6a3a4cf7/src/codec/SkWuffsCodec.cpp#88)

b->meta.closed = s->isAtEnd();

And the semantics of the closed field on the LHS of the assignment is
that true means that we are *definitely* at EOF, "now and in the
future".

However, in Blink code in Chromium's repository, its
blink::SegmentStream class subclasses Skia's SkStream class. Its
isAtEnd() semantics are *possibly* at EOF, as far as we know "at this
point in time":

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/segment_stream.cc;l=82;drc=1feb33707da149b105a135acbea9193e01db0c20

Stepping back a little, blink::ImageDecoder::SetData does have a "bool
all_data_received" argument
(https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/image-decoders/image_decoder.h;l=269;drc=de5356c7681b31e9880823cd8ed46d80ac49131d)
which discriminates between "at this point in time" and "now and in
the future". But that bool isn't passed on to the WTF::SharedBuffer,
blink::SegmentReader, or blink::SegmentStream code.

---

So, either SkWuffsCodec.cpp's use of isAtEnd() or
blink::SegmentStream's implementation of isAtEnd() is incorrect, but I
can't tell which it is just from the SkStream::isAtEnd doc comment (or
studying the SkFILEStream or SkMemoryStream implementations, which
know the stream size at construction).

Is isAtEnd() "possibly EOF, at this point in time" or "definitely EOF,
now and in the future"?

[John Stiles]

I think `isAtEnd` is meant to be a terminal condition; if the possibility exists that more data will arrive, it should probably not be set to true. But that's just my guess, and I don't know if Blink is implemented that way for a reason.

--
You received this message because you are subscribed to the Google Groups "skia-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to skia-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/skia-discuss/CAOeFMNVEdsb2kr2MgQfO7fRzfQJUY2B5LaKUt3bQsYu56U2mjw%40mail.gmail.com.

[scroggo]

John, I believe you are correct. In fact, if you look at SkStream.h, it says:

* Skia streams behave differently. They are effectively synchronous, and will

* always return all N bytes of the request if possible. If they return fewer

* (the read() call returns the number of bytes read) then that means there is

* no more data (at EOF or hit an error). The caller should *not* call again

* in hopes of fulfilling more of the request.

The Blink implementation came later, and doesn't respect the original intent. Maybe it shouldn't have used SkStream, but this was how we were able to get Blink to use SkCodec for GIFs.

[Nigel Tao]

OK, I have sent out https://skia-review.googlesource.com/c/skia/+/772076 which is hopefully straightforward: a SkStream::isAtEnd comment update and a SkWuffsCodec.cpp workaround (and, in some sense, a simplification).

If Skia folks are open to more radical changes, I'm not sure if it's worth the additional effort, but we could deprecate SkStream::isAtEnd. It doesn't look like it actually sees much use. In Chromium, the only call sites (outside of test code and third_party/skia code) are easily replaced (https://crrev.com/c/4987761) and I'd expect we could do something similar for Skia code itself.

[John Stiles]

I would be supportive of removing the API. There isn't a lot going for this one:

- it's rarely used

- the original intent is ambiguous

- in practice, it's easy to replace existing call sites with other APIs

- we can always add new, less ambiguous API if we need something similar down the road

I think someone would just need to own the process of stripping `isAtEnd` out of Blink. Nigel, if you're volunteering, this plan SGTM and I think you can rely on getting +1s from our team as long as the tests still pass :)

On Mon, Oct 30, 2023 at 6:57 AM Nigel Tao <nigel.t...@gmail.com> wrote:

OK, I have sent out https://skia-review.googlesource.com/c/skia/+/772076 which is hopefully straightforward: a SkStream::isAtEnd comment update and a SkWuffsCodec.cpp workaround (and, in some sense, a simplification).

If Skia folks are open to more radical changes, I'm not sure if it's worth the additional effort, but we could deprecate SkStream::isAtEnd. It doesn't look like it actually sees much use. In Chromium, the only call sites (outside of test code and third_party/skia code) are easily replaced (https://crrev.com/c/4987761) and I'd expect we could do something similar for Skia code itself.

--

You received this message because you are subscribed to the Google Groups "skia-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to skia-discuss...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/skia-discuss/cc79a000-6b03-49f1-97cf-5cb6838e2f04n%40googlegroups.com.

[Nigel Tao]

On Tuesday, October 31, 2023 at 1:19:11 AM UTC+11 johns...@google.com wrote:

Nigel, if you're volunteering, this plan SGTM

I'm volunteering but, at a quick look, SkPictureData::parseStreamTag treats "stream has no data (it isAtEnd)" and "stream has data but it's malformed" differently (compare lines 343 and 352 at https://skia.googlesource.com/skia/+/b7ac9da3e5ac34baf5da70b3e37cdbfd898ed54b/src/core/SkPictureData.cpp#343). IIUC, "SkStream::read returns short" is already lumped in with the latter (malformed), not the former (at end).

The "dm --match Serialization" tests actually pass if I simply take out the "if (stream->isAtEnd()) { return false; }" block entirely. But IIUC that's because we don't have test coverage for isAtEnd returning true. Deserialization code, which can be exposed to untrusted input, is also often subtle. I'm not confident that taking out that block is correct, or whether to also change lines 352-358 to "return false".

Any advice?