Improve speed of reading webp into memory (PR #26461)
Maarten
Read the input stream directly into a buffer instead of using a wxMemoryOutputStream.
Increase default temporary buffer size of streams. Reading large files using a small buffer causes significant delays.
See #26457
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26461
Commit Summary
- 300e039 Improve speed of reading webp into memory
- cffa5f8 Increase default temporary buffer size of streams
File Changes
(2 files)
- M src/common/imagwebp.cpp (36)
- M src/common/stream.cpp (2)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26461.patch
- https://github.com/wxWidgets/wxWidgets/pull/26461.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26461)Thanks but, just in case you didn't notice it, this seems to have broken WebP unit tests.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 2 commits.
- d1da88d Improve speed of reading webp into memory
- e3222c6 Increase default temporary buffer size of streams
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
PB
@PBfordev commented on this pull request.
> @@ -448,16 +448,16 @@ size_t wxStreamBuffer::Read(wxStreamBuffer *dbuf)
{
wxCHECK_MSG( m_mode != write, 0, wxT("can't read from this buffer") );
- char buf[BUF_TEMP_SIZE];
+ std::vector<char> buf(BUF_TEMP_SIZE);
This changes the buffer allocation from stack to heap. The function can be called in a loop. Doing something like this usually was not recommended (but it was back then, when the heap was fragmentation-prone and smaller).
I understand that this is probably because of drastically increasing the buffer size. Perhaps a buffer size of the 16 kB would be a sweet spot between stack memory consumption and reading speed? I don't think wxWidgets has a stream benchmark, where this would be easy to test...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -448,16 +448,16 @@ size_t wxStreamBuffer::Read(wxStreamBuffer *dbuf)
{
wxCHECK_MSG( m_mode != write, 0, wxT("can't read from this buffer") );
- char buf[BUF_TEMP_SIZE];
+ std::vector<char> buf(BUF_TEMP_SIZE);
Yes, I mentioned in the commit message that I changed it to use the heap. To prevent the a bigger stack allocation from causing downstream issues. I was not aware of these loop recommendations.
I could make std::vector<char> buf a member of the class so it needs to be allocated only once?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for improving this but I agree that reallocating the buffer for each call to Read() may not be ideal (although I suspect not a big deal either, considering how little all this code is optimized for speed anyhow). I would just increase the size of the existing buffer and do something else only if anybody runs into a problem due to this (which seems very unlikely).
> - wxLogError(_("WebP: Allocating stream buffer failed."));
- }
- return nullptr;
+ // size unknown (for example zip stream), load via wxMemoryOutputStream
+ wxMemoryOutputStream mos;
+ stream.Read(mos);
+
+ wxStreamBuffer* mosb = mos.GetOutputStreamBuffer();
+ size = mosb->GetBufferSize();
+ mosb->Seek(0, wxFromStart);
+
+ buffer.resize(size);
+ size_t readSize = mosb->Read(buffer.data(), size);
+ success = readSize == size;
+ }
+ else {
Trivial:
⬇️ Suggested change- else {
+ else
+ {
> {
- if (verbose)
- {
- wxLogError(_("WebP: Allocating stream buffer failed."));
- }
- return nullptr;
+ // size unknown (for example zip stream), load via wxMemoryOutputStream
Shouldn't we have a helper function in wxInputStream itself doing this, i.e. reading all data into a buffer in the optimal way? This doesn't seem WebP-specific.
> @@ -448,16 +448,16 @@ size_t wxStreamBuffer::Read(wxStreamBuffer *dbuf)
{
wxCHECK_MSG( m_mode != write, 0, wxT("can't read from this buffer") );
- char buf[BUF_TEMP_SIZE];
+ std::vector<char> buf(BUF_TEMP_SIZE);
FWIW I don't think allocating 64KiB on the stack is a problem for any of the platforms supported by wxWidgets, so IMHO we don't need to change this buffer to a vector at all.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 3 commits.
- cd8c7e4 Increase temporary buffer size of streams
- feb0399 Add function to read entire wxInputStream to a buffer
- 29e4187 Improve speed of reading webp into memory
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -448,16 +448,16 @@ size_t wxStreamBuffer::Read(wxStreamBuffer *dbuf)
{
wxCHECK_MSG( m_mode != write, 0, wxT("can't read from this buffer") );
- char buf[BUF_TEMP_SIZE];
+ std::vector<char> buf(BUF_TEMP_SIZE);
Ok, I've reverted the changes to only increasing the buffer size.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> {
- if (verbose)
- {
- wxLogError(_("WebP: Allocating stream buffer failed."));
- }
- return nullptr;
+ // size unknown (for example zip stream), load via wxMemoryOutputStream
Added it. I hope the API is fine like this.
I kept using the wxMemoryOutputStream. I could also replace it with the contents of wxInputStream::Read(wxOutputStream& stream_out), but write to (and keep resizing) the vector instead. Any preference?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> {
- if (verbose)
- {
- wxLogError(_("WebP: Allocating stream buffer failed."));
- }
- return nullptr;
+ // size unknown (for example zip stream), load via wxMemoryOutputStream
Actually, I will replace it. Because both cases (size known/unknown) can use the same code then.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 2 commits.
- e8899c6 Add function to read entire wxInputStream to a buffer
- f7d3f72 Improve speed of reading webp into memory
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, but I think there is a problem with the new Read() overload return value, please let me know if I'm missing something.
> @@ -134,6 +134,10 @@ class WXDLLIMPEXP_BASE wxInputStream : public wxStreamBase
// when EOF is reached or an error occurs
wxInputStream& Read(wxOutputStream& streamOut);
+ // copy the entire contents of this stream into buffer, stopping only
+ // when EOF is reached or an error occurs
+ bool Read(std::vector<char>& buffer);
I wonder if we're not going to have problems with people (possibly including ourselves) wanting to use this with unsigned char or uint8_t or std::byte... But I don't see any nice way to generalize it, we probably don't want to make the entire function template and extracting a type-independent implementation of it would require using some VectorHolder class with virtual functions wrapping data() and resize() which would be quite ugly.
So I guess for now this is the best we can do, except that perhaps we should use unsigned char?
> @@ -648,6 +648,15 @@ class wxInputStream : public wxStreamBase
*/
wxInputStream& Read(wxOutputStream& stream_out);
+ /**
+ Read all data from the stream and stores it in the specified buffer.
+ Data is read until the end of the input stream, or until an error occurs.
+ The buffer will be resized to the number of bytes read.
+
It would be good to document that it returns false if nothing was read, even if no error occurred which is not obvious but which is how the code seems to behave (at least if I'm reading the code correctly).
> + if ( !bytes_read ) + break; + + if ( buffer.size() < (lastcount + bytes_read) ) + buffer.resize(lastcount + bytes_read); + memcpy(buffer.data() + lastcount, buf, bytes_read); + + lastcount += bytes_read; + } + } + + m_lastcount = lastcount; + + buffer.resize(m_lastcount); + + return !buffer.empty();
Actually I don't think it's the correct return value. In the single read case it should return lastcount == stream_size and in the other one it should return false instead of break (possibly after setting m_lastcount).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -134,6 +134,10 @@ class WXDLLIMPEXP_BASE wxInputStream : public wxStreamBase
// when EOF is reached or an error occurs
wxInputStream& Read(wxOutputStream& streamOut);
+ // copy the entire contents of this stream into buffer, stopping only
+ // when EOF is reached or an error occurs
+ bool Read(std::vector<char>& buffer);
I tried uint8_t initially (because libwebp uses it) but that didn't work everywhere without extra includes.
I'm using char because it is already used by other functions in the classes in stream.h, while unsigned is not used at all.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -134,6 +134,10 @@ class WXDLLIMPEXP_BASE wxInputStream : public wxStreamBase
// when EOF is reached or an error occurs
wxInputStream& Read(wxOutputStream& streamOut);
+ // copy the entire contents of this stream into buffer, stopping only
+ // when EOF is reached or an error occurs
+ bool Read(std::vector<char>& buffer);
I still think char is a bad choice for the "byte" type due to the possibility of introducing difficult to notice bugs when using it and I don't think it's used anywhere in th public API (char itself is, but not char*), so I'd still consider using unsigned char (a.k.a. wxUint8 a.k.a. wxByte) but I'm leaving the choice to you.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> + if ( !bytes_read ) + break; + + if ( buffer.size() < (lastcount + bytes_read) ) + buffer.resize(lastcount + bytes_read); + memcpy(buffer.data() + lastcount, buf, bytes_read); + + lastcount += bytes_read; + } + } + + m_lastcount = lastcount; + + buffer.resize(m_lastcount); + + return !buffer.empty();
I think it should return true if the entire input stream is written into the buffer. I'll update the code.
The second case will always break, it keeps reading until nothing is available. In this case I'll return true when no error happened.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.