Opened 15 years ago

Closed 15 years ago

Last modified 12 months ago

#8396 closed patch

smush_player performance patch

Reported by: SF/ole00 Owned by:
Priority: normal Component: Engine: SCUMM
Keywords: Cc:



I have an update for the scumm smush_player and chunk
class. The old smush player was fairly excelent on fast
computers wheere seek / and transfer times are very
short. On the other hand if smush player was not fed
properly right from the start (mostly happens on
platforms where there is no read-ahead file caching,
and on platforms where data transfer rate is slow -
flash disks etc.) then
the playback performace suffer. Another glitch
concerned the time-compression on the start of the
movie (first 2 or 3 seconds of the movie were missing -
or played back at once with stacked voice streams) is
also solved.

This fix/update adds read-ahead caching to the chunk
class (I have created CachedFileChunk from the
FileChunk) and slightly modifies the smush_player.

I'm sorry that the source is attached here, but I have
no other easier way how to contribute.

Hope it will be usefull - if not, never mind :).

BTW. Why the chunk classes are passed as 'instance
copy' (Chunk &b) in the smush/saud methods and not as
the pointers (Chunk *b)? Maybe I'm wrong but this way
the system creates instance copies of the chunks each
time it is passed to another method which might be imho
quite memory/time consuming...


The source code is attached bellow....

Ticket imported from: #1115053. Ticket imported from: patches/501.

Attachments (1)

smush-speed.txt (7.7 KB ) - added by SF/ole00 15 years ago.
smush/chunk performance update source code

Download all attachments as: .zip

Change History (7)

by SF/ole00, 15 years ago

Attachment: smush-speed.txt added

smush/chunk performance update source code

comment:1 by fingolfin, 15 years ago

Better learn some C++ ;-). The "&" operator here indicates a
"reference", which is, to simplify the matter a bit, kind of a "hidden"
pointer. So no copy is performed, and no performance lost.

comment:2 by fingolfin, 15 years ago

Some remarks:

* Please submit patches in the "patch" format. Any CVS program
should be able to generate such a patch for you, using its "diff"
command. You don't tell us what OS you are using, so I won't
go into details.

* You added a DW_LE_BE macro. Don't. Use SWAP_BYTES_32
or bypass getDword and call read() directly.

comment:3 by SF/ole00, 15 years ago

I'm sorry for the source format, but Im' still working with
the old 0.7.0 version and due to the testing/researching of
the code functionality I have added many tracing printfs
which would make a total diff-mess...
BTW. I found the bug in the submited CachedFileChunk that
shows up when the main (parent) chunk is deleted earlier
than it's child subchunk (occurs in the Insane
repositions). That way the system frees the memory of the
chunk and lately allocates the memory space to the new
chunk, but the sub chunk still holds memory pointer now
pointing to the new chunk (and during destruction sets the
wrong cache data...). This behaviour is harmless for
FileChunk, because theese are not sharing/updating the cache
data.... Anyway I fixed it by applying id's for parent-child
so child would update cache positions for true-parent chunk
About the os I'm using the win32 os and working on the ps2
port which has no real os system. Generally most of the
scummvm engines suppose that there is an underlying os that
is caching the data files. That leads to some performance
slowdowns (calling read of size 1 byte is not rare occasion
:-) that I'm dealing right now. I plan to create some simple
cache solution and put it nearby the file.cpp.

comment:4 by fingolfin, 15 years ago

Well, w/o a patch made against current CVS, this patch won't
make it even to the "testing" stage, so unless you provide one,
I am afraid it has little chance of being accepted :-/

Regarding file caching: yeah, the File class is written around the
assumption that the OS or the backend author provides some kind of
sane implementation of fopen/fread/fclose.
If that's a serious problem, we can think about making the File class
customizable by backends. But this should be done in a clean and
transparent fashion, well designed, and not as an after thought add-on
cruft thingy... :-)

comment:5 by fingolfin, 15 years ago

Status: newclosed

comment:6 by digitall, 12 months ago

Component: Engine: SCUMM
Note: See TracTickets for help on using tickets.