Icon issue in 2.9.0

[Francesco Montorsi]

[cc'ing to Francesco as I'm not sure he's already subscribed to this list]

I read wx-users through gmane and so I didn't subscribe the new list; gmane
works fine for now ;)
Anyway please cc me also for future issues that appear on wx-users as for the
next months I'll have few free time and so I typically don't read wx-users...

MF> I just grabbed 2.9.0 from SVN and code that worked under 2.8.9 is now
MF> failing (VS 2008, Vista). I am trying to load a .ico file from a Zip stream.
MF> It seems to just be icon files, as I can load any other image type fine.
This was broken by http://svn.wxwidgets.org/viewvc/wx?view=rev&revision=58080
which was apparently necessary to fix some other bug

exactly

but breaking this is
still unacceptable, of course. I hope Francesco can look into this because
personally I don't see any other solution than reverting this change right
now.

reverting the change is not the right thing. Everytime wx code calls a function
which possibly returns an error/success code, it should check if the call was ok.
Simply disregarding the error/success return code is not a good thing, specially
if the code following that call assumes that the operation was fine (maybe not
all calls changed by r58080 assumed it, but most did).

Here the problem is that, as documented, wxZipInputStream is not a seekable
stream. wxICOHandler, except for the initial SeekI(0), seems to need seekable
streams only in line 1323 of imagbmp.cpp, i.e. when loading a specific icon from
a multi-icon .ico file.

The initial SeekI(0) is present in almost all wx*Handler classes.
I think it's necessary because loading an image implies that
wxICOHandler::DoCanRead(), which moves the "current position" in the stream, is
called before wxICOHandler::DoLoadFile.

I see two possible solutions:
1) reset the current position in the stream in the wxICOHandler::DoCanRead()
function, instead of doing it in DoLoadFile (so that if you call directly
DoLoadFile, it may work also on non-seekable streams).

2) avoid trying to seek the stream directly in SeekI(), if that's possible.
Something like:

Index: src/common/stream.cpp
===================================================================
--- src/common/stream.cpp (revisione 60787)
+++ src/common/stream.cpp (copia locale)
@@ -923,6 +923,14 @@
if (m_lasterror==wxSTREAM_EOF)
m_lasterror=wxSTREAM_NO_ERROR;

+ // avoid unnecessary seek operations: this is not only an optimization; it also
+ // allows code which makes calls to SeekI() which in most cases are unnecessary
+ // (e.g. image handlers) to (possibly) work also with non-seekable streams...
+ if ((mode == wxFromStart && TellI() == pos) ||
+ (mode == wxFromCurrent && pos == 0) ||
+ (mode == wxFromEnd && GetLength() != wxInvalidOffset && TellI() ==
GetLength()-pos))
+ return TellI();
+
/* RR: A call to SeekI() will automatically invalidate any previous
call to Ungetch(), otherwise it would be possible to SeekI() to
one position, unread some bytes there, SeekI() to another position

Could you test this change and see if your icon gets loaded?
If the code works then it needs to be copied also in wxBufferedInputStream (see
the initial comment in wxInputStream::SeekI).

Francesco



PS: This issue remembers me of http://trac.wxwidgets.org/ticket/10373 which in
the last comment proposes a further possible enhancement to wxInputStream::SeekI().

[Matthew Fanto]

Post by Francesco Montorsi
Could you test this change and see if your icon gets loaded?
If the code works then it needs to be copied also in wxBufferedInputStream
(see the initial comment in wxInputStream::SeekI).

Yes, the patch did correct the problem and it now works. The icon is loaded,
and no errors are given. Thank you very much.

Best,
Matthew Fanto

[Vadim Zeitlin]

On Thu, 28 May 2009 22:12:02 +0200 Francesco Montorsi <***@yahoo.it> wrote:

FM> reverting the change is not the right thing.

Yes, I definitely agree with this but unfortunately I didn't have time to
do what you did and find the right thing to do.

FM> Here the problem is that, as documented, wxZipInputStream is not a
FM> seekable stream. wxICOHandler, except for the initial SeekI(0), seems
FM> to need seekable streams only in line 1323 of imagbmp.cpp, i.e. when
FM> loading a specific icon from a multi-icon .ico file.

Even this could be avoided as currently we have

wxFileOffset iPos = stream.TellI();
...
stream.SeekI(iPos + wxUINT32_SWAP_ON_BE(pCurrentEntry->dwImageOffset),
wxFromStart);

which could be rewritten as

stream.SeekI(wxUINT32_SWAP_ON_BE(pCurrentEntry->dwImageOffset),
wxFromCurrent);

and seeking a positive amount from the current position could be
implemented for any stream by just reading and discarding the given amount
of data.

FM> The initial SeekI(0) is present in almost all wx*Handler classes.

Yes, and IMO it's wrong to have it there. CanRead() really shouldn't
modify the stream position in the first place and so it should rewind the
stream back if it changed it. As for LoadFile(), it shouldn't do anything
at all with the stream position.

FM> I think it's necessary because loading an image implies that
FM> wxICOHandler::DoCanRead(), which moves the "current position" in the stream, is
FM> called before wxICOHandler::DoLoadFile.

This is only necessary if you accept that DoCanRead() changes the current
position which doesn't make much sense: why should testing something change
the object being tested?

FM> I see two possible solutions:
FM> 1) reset the current position in the stream in the wxICOHandler::DoCanRead()
FM> function, instead of doing it in DoLoadFile (so that if you call directly
FM> DoLoadFile, it may work also on non-seekable streams).

This is the one which I prefer, by far.

FM> 2) avoid trying to seek the stream directly in SeekI(), if that's possible.
FM> Something like:
FM>
FM> Index: src/common/stream.cpp
FM> ===================================================================
FM> --- src/common/stream.cpp (revisione 60787)
FM> +++ src/common/stream.cpp (copia locale)
FM> @@ -923,6 +923,14 @@
FM> if (m_lasterror==wxSTREAM_EOF)
FM> m_lasterror=wxSTREAM_NO_ERROR;
FM>
FM> + // avoid unnecessary seek operations: this is not only an optimization; it also
FM> + // allows code which makes calls to SeekI() which in most cases are unnecessary
FM> + // (e.g. image handlers) to (possibly) work also with non-seekable streams...
FM> + if ((mode == wxFromStart && TellI() == pos) ||
FM> + (mode == wxFromCurrent && pos == 0) ||
FM> + (mode == wxFromEnd && GetLength() != wxInvalidOffset && TellI() ==
FM> GetLength()-pos))
FM> + return TellI();

I don't have any real objections to this patch but IMO it should be just
an optimization and not something that we rely on. Also, TellI() doesn't
work for all streams neither so it still won't help you if you try to load
an image from e.g. a socket stream.

Regards,
VZ

[Francesco Montorsi]

Post by Vadim Zeitlin
FM> Here the problem is that, as documented, wxZipInputStream is not a
FM> seekable stream. wxICOHandler, except for the initial SeekI(0), seems
FM> to need seekable streams only in line 1323 of imagbmp.cpp, i.e. when
FM> loading a specific icon from a multi-icon .ico file.
Even this could be avoided as currently we have
wxFileOffset iPos = stream.TellI();
...
stream.SeekI(iPos + wxUINT32_SWAP_ON_BE(pCurrentEntry->dwImageOffset),
wxFromStart);
which could be rewritten as
stream.SeekI(wxUINT32_SWAP_ON_BE(pCurrentEntry->dwImageOffset),
wxFromCurrent);
and seeking a positive amount from the current position could be
implemented for any stream by just reading and discarding the given amount
of data.

indeed; that's what was suggested in http://trac.wxwidgets.org/ticket/10373

Post by Vadim Zeitlin
FM> The initial SeekI(0) is present in almost all wx*Handler classes.
Yes, and IMO it's wrong to have it there. CanRead() really shouldn't
modify the stream position in the first place and so it should rewind the
stream back if it changed it. As for LoadFile(), it shouldn't do anything
at all with the stream position.

I agree

Post by Vadim Zeitlin
FM> I think it's necessary because loading an image implies that
FM> wxICOHandler::DoCanRead(), which moves the "current position" in the stream, is
FM> called before wxICOHandler::DoLoadFile.
This is only necessary if you accept that DoCanRead() changes the current
position which doesn't make much sense: why should testing something change
the object being tested?

it shouldn't in fact...

Post by Vadim Zeitlin
FM> 1) reset the current position in the stream in the wxICOHandler::DoCanRead()
FM> function, instead of doing it in DoLoadFile (so that if you call directly
FM> DoLoadFile, it may work also on non-seekable streams).
This is the one which I prefer, by far.

I've implemented it in r60852, together with other related changes (e.g. the
same problem is present also for GetImageCount()).


Francesco

[Francesco Montorsi]

Hi,

On Thu, May 28, 2009 at 4:12 PM, Francesco Montorsi
Could you test this change and see if your icon gets loaded?
If the code works then it needs to be copied also in
wxBufferedInputStream (see the initial comment in wxInputStream::SeekI).
Yes, the patch did correct the problem and it now works. The icon is
loaded, and no errors are given. Thank you very much.

good, thanks for the feedback.

I've now fixed the problem in a more extensive way in wx trunk. If you can test
the changes and report any problem, it would be very nice. TIA!

Francesco