Switch image handling in HTML from wxImage to wxBitmapBundle (PR #26474)
Randalphwa
Primary reason is to allow SVG images in HTML pages, with the added bonus of multi-resolution png files being bundled together for better scaling on high DPI displays.
I also added a toolbar to the html/test sample just to make it easier to move forward/back including disabling/enabling the buttons depending on whether any history was available.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26474
Commit Summary
- dff45c6 Switch to using wxBitmapBundle instead of wxImage
- 06d3654 Add a toolbar for moving back/forward
- b5739f8 Update wxHtmlWindow interface docs
File Changes
(4 files)
- M interface/wx/html/htmlwin.h (5)
- M samples/html/test/test.cpp (26)
- M src/html/m_image.cpp (171)
- M src/html/m_layout.cpp (35)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26474.patch
- https://github.com/wxWidgets/wxWidgets/pull/26474.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.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#26474)Turning this into a draft -- seeing a discrepancy between what lunasvg should be displaying versus what is actually displaying...
—
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.
Looks good, thanks!
I'd just like to avoid duplicating the code reading SVGs from stream, could you please do this? TIA!
> bool readImg = true;
- if ( m_windowIface &&
- (input->GetLocation().Matches(wxT("*.gif")) ||
- input->GetLocation().Matches(wxT("*.GIF"))) )
+ wxString loc = input->GetLocation();
+
+ // SVG image path
+ if ( loc.Matches("*.svg") || loc.Matches("*.SVG") )
+ {
+#ifdef wxHAS_SVG
+ // Read the entire stream into a buffer for SVG
+ wxMemoryBuffer svgBuf;
+ for ( ;; )
+ {
+ char tmp[4096];
We have just seen in #26461 that using 64KiB buffer is much faster than using 4KiB, so I think we should change the size here too. Or maybe just use Read(std::vector<char>& buffer) overload once the other PR is merged.
> + // SVG image path
+ if ( loc.Matches("*.svg") || loc.Matches("*.SVG") )
+ {
+#ifdef wxHAS_SVG
+ // Read the entire stream into a buffer for SVG
+ wxMemoryBuffer svgBuf;
+ for ( ;; )
+ {
+ char tmp[4096];
+ s->Read(tmp, WXSIZEOF(tmp));
+ const size_t n = s->LastRead();
+ if ( n == 0 )
+ break;
+ svgBuf.AppendData(tmp, n);
+ }
+ wxBitmapBundle svgBundle = wxBitmapBundle::FromSVG(
Or maybe add FromSVG() overload taking a stream?
—
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.
Randalphwa
@Randalphwa commented on this pull request.
> + // SVG image path
+ if ( loc.Matches("*.svg") || loc.Matches("*.SVG") )
+ {
+#ifdef wxHAS_SVG
+ // Read the entire stream into a buffer for SVG
+ wxMemoryBuffer svgBuf;
+ for ( ;; )
+ {
+ char tmp[4096];
+ s->Read(tmp, WXSIZEOF(tmp));
+ const size_t n = s->LastRead();
+ if ( n == 0 )
+ break;
+ svgBuf.AppendData(tmp, n);
+ }
+ wxBitmapBundle svgBundle = wxBitmapBundle::FromSVG(
I'm going to wait until #26461 gets checked in. With SVG in an HTML file, I'm concerned about a decompression bomb blowing up the caller, so if ReadStream doesn't have a buffer size limit setting, then I wouldn't risk it for SVG inside HTML. We don't currently support .svgz format, but that could definitely be supported in the future (and probably should be) -- and that would be the biggest risk for a decompression bomb with an svgz file from an untrusted source.
—
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.
> + // SVG image path
+ if ( loc.Matches("*.svg") || loc.Matches("*.SVG") )
+ {
+#ifdef wxHAS_SVG
+ // Read the entire stream into a buffer for SVG
+ wxMemoryBuffer svgBuf;
+ for ( ;; )
+ {
+ char tmp[4096];
+ s->Read(tmp, WXSIZEOF(tmp));
+ const size_t n = s->LastRead();
+ if ( n == 0 )
+ break;
+ svgBuf.AppendData(tmp, n);
+ }
+ wxBitmapBundle svgBundle = wxBitmapBundle::FromSVG(
To handle this we'd need to add something like maxSize = 0 (meaning none by default) parameter to the Read() overload being added there and probably also provide some way of setting it for SVGs explicitly as I don't think we can choose one max value fitting all use cases.
—
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.
Randalphwa
@Randalphwa commented on this pull request.
> + // SVG image path
+ if ( loc.Matches("*.svg") || loc.Matches("*.SVG") )
+ {
+#ifdef wxHAS_SVG
+ // Read the entire stream into a buffer for SVG
+ wxMemoryBuffer svgBuf;
+ for ( ;; )
+ {
+ char tmp[4096];
+ s->Read(tmp, WXSIZEOF(tmp));
+ const size_t n = s->LastRead();
+ if ( n == 0 )
+ break;
+ svgBuf.AppendData(tmp, n);
+ }
+ wxBitmapBundle svgBundle = wxBitmapBundle::FromSVG(
Hmm, I need to do a deeper dive into this. A compression bomb can also be in webp as well -- I gather it's something Google is trying to work around. I think for an SVG the only way to get a compression bomb is via .svgz -- in that case, we would presumably be calling 'wxZlibInputStream(wxInputStream* stream)' -- which would mean that trying to read the entire stream at once could blow up the app unless there was a limit with an error return if that limit was succeeded. For SVG that would probably be around 10 MB as the largest valid SVG -- no idea what webp, jpeg, png, tiff, etc. are -- Tiff I know can easily be over 1GB in size and still be valid.
—
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#26474)I'm marking this as draft because I think you still plan to change it — please let me know if this was wrong or reset the draft status when it's done. TIA!
—
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.
Randalphwa
@Randalphwa pushed 6 commits.
- 8b9002e Add wxBitmapBundle::FromSVG(wxInputStream& stream overload
- a23768a Switch to using the new wxBitmapBundle::FromSVG(wxInputStream& stream
- e200005 Add local file support for compressed .svgz files
- 7fe579c Add html stream support for compressed .svgz files
- f3883dd Create sample HTML showcasing regular and compressed SVG images
- 64da5f8 Minor formatting changes
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa pushed 1 commit.
- 3bab5dd Fix placement of #endif conditionals
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#26474)Still a draft -- I need to update documentation, and fix the Check Mixed EOL test that doesn't realize a .svgz is a binary file. However, the updated htmltest is now able to display svg_test.htm which has both a regular and compressed .svg/.svgz file referenced in the HTML file, so support for .svgz is definitely working, as well as the new FromSVG that takes an input stream per @vadz recommendation (which made implementing support for .svgz trivial).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Randalphwa
@Randalphwa pushed 4 commits.
- 6a5c1d4 Add .svgz to list of binary files (compressed SVG)
- ff6c22d Fix unused variable when !wxHAS_SVG
- 4c149bd Added documentation about FromSVG(wxInputStream
- 57112dd Added documentation about SVG in HTML files
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks, all this looks good to me but, just to confirm before merging: the removed Mac-specific code is really not needed any longer, i.e. high DPI bitmaps are still used under Mac even without it?
Also, a minor point but I think the new code doesn't compile if wxUSE_ZLIB && !wxUSE_STREAMS, but, again, this is very minor and I'm not sure if this build configuration is supported even now.
And another very minor point is that we use both s.Lower().EndsWith(".svgz") and s.Matches("*.gif") || s.Matches("*.GIF") which just seems gratuitously inconsistent, I'd standardize on one or the other (or, alternatively, wxFileName(s).GetExt().Lower() == "foo" which is the most correct, even also the most cumbersome).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Randalphwa
Randalphwa left a comment (wxWidgets/wxWidgets#26474)Since everything is now going through our wxBundle code, there should no longer be any reason to special case Mac -- but I'll check that to be sure.
It may not be supported, but I agree with you that adding the !wxUSE_STREAMS is both correct, and avoids a potential maintenance issue down the road.
I also agree that 'wxFileName(s).GetExt().Lower()' would be preferred -- replacing the *gif handling and using the same system -- it's a bit more processing, but this isn't in a loop, so it won't have any realistic impact on performance.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.