RetroArch icon indicating copy to clipboard operation
RetroArch copied to clipboard

rzipstream_tell: Fix missing tell for non compressed files

Open pasnox opened this issue 1 year ago • 8 comments

The tell call would always return the compressed stream position.

pasnox avatar Aug 11 '22 21:08 pasnox

When the zip isn't compressed, isn't that going to return the incorrect position because it will not factor the header and its metadata?

ghost avatar Aug 11 '22 22:08 ghost

https://github.com/libretro/RetroArch/blob/908667d97ac1ad370ee61665f777cd996e6ba2aa/libretro-common/vfs/vfs_implementation.c#L598-L601

Is this even correct? This appears to be only reachable if mmap fails: https://github.com/libretro/RetroArch/blob/908667d97ac1ad370ee61665f777cd996e6ba2aa/libretro-common/vfs/vfs_implementation.c#L466-L468 (this won't clear the RFILE_HINT_UNBUFFERED flag set here: https://github.com/libretro/RetroArch/blob/908667d97ac1ad370ee61665f777cd996e6ba2aa/libretro-common/vfs/vfs_implementation.c#L321-L325

ghost avatar Aug 11 '22 23:08 ghost

Well, I did not added a dependency on file stream. It existed already. I just provide a fix for tell. I suppose transparent file stream support was added to support both compressed / non compressed playlists without a burden, which is not a bad idea. Now I did not checked deeper the quality of the support.

pasnox avatar Aug 12 '22 06:08 pasnox

Alright, point taken on the already existing filestream dependencies. No problem then. I will remove the prior complaint. Let's just focus on covering the existing issues pointed out by @Cthulhu-throwaway instead.

LibretroAdmin avatar Aug 12 '22 07:08 LibretroAdmin

When the zip isn't compressed, isn't that going to return the incorrect position because it will not factor the header and its metadata?

I've been told this is not a real concern because of the following:

'If the file isn't compressed, then there is no header - so there will be no offset to correct'.

VFS comments though might be worthy of being addressed.

LibretroAdmin avatar Aug 12 '22 15:08 LibretroAdmin

'If the file isn't compressed, then there is no header - so there will be no offset to correct'.

Haven't checked if the library packs archives (ZIPs that are not compressed) as "compressed", but non-compressed archives do absolutely have a header and tree metadata.

>>> from pathlib import Path
>>> with Path("test.zip").open("rb") as f:
...    f.read()
...
b'PK\x03\x04\n\x00\x00\x00\x00\x00,{\x0cU\x04\x8df\xbc\x1a\x00\x00\x00\x1a\x00\x00\x00\x08\x00\x00\x00test.txtNON COMPRESSED TEST TEXT\r\nPK\x01\x02?\x00\n\x00\x00\x00\x00\x00,{\x0cU\x04\x8
df\xbc\x1a\x00\x00\x00\x1a\x00\x00\x00\x08\x00$\x00\x00\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00test.txt\n\x00 \x00\x00\x00\x00\x00\x01\x00\x18\x00\x18;?\xe3x\xae\xd8\x01\xdd\xdd\xf
2\xd5x\xae\xd8\x01\xdd\xdd\xf2\xd5x\xae\xd8\x01PK\x05\x06\x00\x00\x00\x00\x01\x00\x01\x00Z\x00\x00\x00@\x00\x00\x00\x00\x00'

ghost avatar Aug 12 '22 18:08 ghost

'If the file isn't compressed, then there is no header - so there will be no offset to correct'.

Haven't checked if the library packs archives (ZIPs that are not compressed) as "compressed", but non-compressed archives do absolutely have a header and tree metadata.

>>> from pathlib import Path
>>> with Path("test.zip").open("rb") as f:
...    f.read()
...
b'PK\x03\x04\n\x00\x00\x00\x00\x00,{\x0cU\x04\x8df\xbc\x1a\x00\x00\x00\x1a\x00\x00\x00\x08\x00\x00\x00test.txtNON COMPRESSED TEST TEXT\r\nPK\x01\x02?\x00\n\x00\x00\x00\x00\x00,{\x0cU\x04\x8
df\xbc\x1a\x00\x00\x00\x1a\x00\x00\x00\x08\x00$\x00\x00\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00test.txt\n\x00 \x00\x00\x00\x00\x00\x01\x00\x18\x00\x18;?\xe3x\xae\xd8\x01\xdd\xdd\xf
2\xd5x\xae\xd8\x01\xdd\xdd\xf2\xd5x\xae\xd8\x01PK\x05\x06\x00\x00\x00\x00\x01\x00\x01\x00Z\x00\x00\x00@\x00\x00\x00\x00\x00'

What it mean by not compressed, is that it's just a bare non archive file (not a RZIP file), then it has no header and the code handle just that - treat a non rzip file opened with rzip stream as a plain file stream.

pasnox avatar Aug 12 '22 19:08 pasnox

What it mean by not compressed, is that it's just a bare non archive file (not a RZIP file), then it has no header and the code handle just that - treat a non rzip file opened with rzip stream as a plain file stream.

Should be renamed from is_compressed to is_archive then, imo. Will avoid confusions like these.

ghost avatar Aug 12 '22 20:08 ghost