#8401 closed patch
Less memory-hungry MPEG playback
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Video |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
This is an attempt to adapt some code from SDL to make the MPEG cutscene player less memory hungry.
With our current code, we allocate a lookup table of 256 * (BITDEPTH + 1)^2 OverlayColor entries. I believe this comes out to about 8 MB of data; data that is shared between all instances of the cutscene player, so it's not freed between cutscenes. With the new code, two lookup tables are allocated. One that's 4 KB (assuming an int is four bytes) and one that's 9 KB.
I haven't touched the 8-bit case, but I think the lookup table for that one is around 70 KB so it's not too bad.
Note that this is still fairly untested code. There could be out-of-bounds memory access, and I haven't had much time to compare the output or the performance to the current version yet. And I've probably only tried the 555 case, not the 565 case. So if anyone else wants to try it, I'd be grateful.
Ticket imported from: #1155731. Ticket imported from: patches/506.
Attachments (2)
Change History (14)
by , 19 years ago
| Attachment: | animation.diff added |
|---|
comment:1 by , 19 years ago
I forgot to say, I'm well aware that the changes I made do not conform with ScummVM naming standards. But since that could be said about much of the present code in that class, I figured such changes could wait until later.
comment:2 by , 19 years ago
Some preliminary testing didn't turn up any Valgrind errors. The new code may be a tiny bit slower than the old one, but if so it doesn't appear to be by much.
comment:3 by , 19 years ago
Some comments:
* while of course we should and will credit the authors of the code this is based on, we gotta be careful about inserting other people's licensing terms into our code. If we use that code, it'll be GPL -- period. The comments you add to graphics/animation.cpp imply a BSD-style licensing for that file, or at least for "portions of that file". Of course we can dual license the code, but I am worrying a bit about the full legal impact, so we should double check this... [I am a bit touchy to such legal issues, partially due to my work in Fink]
* I tested the code on my OS X machine. In 565 mode. Seems to work perfectly fine -> great work!
comment:4 by , 19 years ago
I'm not sure how to handle those credits. The reason I put them there was that they are present in SDL's src/video/SDL_yuv_sw.c and that I would never have been able to get the patch to work without copying from that file.
I've attached an updated patch, but the only difference between that and the first one is that I've changed it to use smaller integer types where it appeared safe to do so. (So now the lookup tables are 2 KB and 4.5 KB instead.)
An alternative would of course be to expose enough of SDL's "YUV overlay" API that ScummVM could use it. The problems I see with that is that it places a much heavier burden on the other backend porters (either that or they'll have to keep using the current memory hog), and that it causes problems with the scalers since SDL can only scale these overlays to 1x and 2x as far as I can tell.
comment:5 by , 19 years ago
By the way, the original source code appears to be "mpeg_play", which was to be expected since there's a comment in the SDL source code that refers to it.
It can be found at http://bmrc.berkeley.edu/frame/research/mpeg/mpeg_play.html and the code in question is in the file 16bit.c. That also appears to be where the three copyright notices are coming from.
I guess I could look around to see if I can find any functionally similar code under different licenses, if that's desirable...
comment:6 by , 19 years ago
I can think of three projects that may have some useful code to borrow:
MPlayer - postproc/yuv2rgb.c Xine - src/video_out/yuv2rgb.c GStreamer (gst-plugins) - gst/colorspace/yuv2rgb.c
Of the three, GStreamer is the one that looks most easy to read to me. In fact, it contains an #if 0:ed copy of what looks like the mpeg_play code - sans copyright notices - presumably for reference.
The Xine and MPlayer code look like they may have evolved from the same base, with no visible traces of mpeg_play. MPlayer makes heavier use of the C preprocessor.
comment:7 by , 19 years ago
You misunderstood me -- it's perfectly OK to use the code as you did. My sole concern was regarding how the copyright of that additional code is handled. Right now, you include a comment which starts "This function, and the next one [...]", followed by three BSD-style license phrases. The thing is, the claims talk about "the software", but of course we do *not* want to apply those terms to all of ScummVM.
It might be sufficient to add the following sentence at the top: "In the following, the term 'software' is used to refer to the code of the next two functions". Or something alike...
Using other code which is also derived from the same source but doesn't state the copyright clearly isn't any better, rather, it'd be worse ...
Oh and two other things: * it's hard to predict performance, of course, but I guess the fact that tables sized a few KB only will fit into the cache; an 8 MB table usually won't; so even if there is some more overhead involved to compute the end value, the reduced memory usage propably will help a lot more than this costs...
* cleaning up the naming convention stuff later on is alright by me...
comment:8 by , 19 years ago
I'm no license expert, so the best I can come up with on short notice is this:
"The YUV to RGB conversion code is derived from SDL's YUV overlay code, which in turn appears to be derived from mpeg_play. The following copyright notices have been included in accordance with the original license. Please note that the term "software" in this context only applies to the two functions buildLookup() and plotYUV() below."
Do you think that's good enough?
(By the way, if you don't count the #if 0:ed part, the GStreamer code is almost entirely different from the mpeg_play code as far as I can tell.)
comment:11 by , 19 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:12 by , 5 years ago
| Component: | → Video |
|---|
Patch against a March 3 CVS snapshot