#9141 closed patch
VideoDecoder Rewrite
| Reported by: | SF/mthreepwood | Owned by: | SF/mthreepwood |
|---|---|---|---|
| Priority: | normal | Component: | Video |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
I've rewritten VideoDecoder because it was extremely limited as to what you could do with it. So, here's what this patch includes:
1) Add support for >8bpp images 2) Remove reliance on delaying to get to the next frame 3) As a result of 2, videos can now be run in the background much more easily 4) Rewrote the sync code to remove the reliance on *100 for all time values (and therefore removes some of the rounding) 5) Add support for non-integer frame rates, fixes the GK1 credits video (0.75fps) 6) Add support for the possible inclusion of non-fixed frame rate videos (such as moving QuickTime to the common code so it can be used in SCI as well as Mohawk) 7) Fixed a case in SmackerDecoder where it would call exit(1) (Why was that even in there????)
All video formats and engines (minus SAGA2) have been tested. However, only the intro of Tucker has been tested and not the full game. The non-interactive demo works fine too.
Other possible issues: In Sword1/Sword2 the subtitle palette indices for black and white are set to 0 and 255, respectively and don't search the current palette for these. This seems to be working ok, but we can add search code again if need-be.
Thanks to DrMcCoy for assistance and testing.
Ticket imported from: #2963496. Ticket imported from: patches/1246.
Attachments (7)
Change History (35)
by , 15 years ago
| Attachment: | video_decoder_rewrite.diff added |
|---|
comment:1 by , 15 years ago
Forgot to mention this: The patch also allows 8bpp videos to be displayed in 16bpp mode, providing that the calling code converts the image from 8bpp to 16bpp. Previously, VideoDecoder would apply the palette to the system no matter what, which would fail in 16bpp mode.
comment:2 by , 15 years ago
Thanks for your patch, seems to contain some pretty useful changes based on your description at least. Some quick comments:
* This patch will have to wait till after 1.1.0 has been branched
* Why are many frame values in Tucker changed by +1?
* The patch file is 130 kb, wow. That makes it a bit tough to review this. Esp. since many changes seem to be simple refactoring / renaming, and thus the "real" changes are hidden. In particular, you seem to have renamed _fileStream to _stream. This makes sense, but causes lots of "noise" in the patch. It would be much better if this was split into a separate patch (which could be reviewed and applied independently).
* Using floating point arithmetic is quite problematic, I am afraid. It has to be expensively emulated on many systems, e.g. ARM. May I suggest using Audio::Timestamp instead? This also avoid any rounding errors when dealing with frames and milliseconds.
comment:3 by , 15 years ago
Question: Does Audio::Timestamp work with non-integer framerates (for example, 0.75fps in GK1)? From what I've seen, that's not really possible, is it?
comment:4 by , 15 years ago
I see. So you have 75 frames per 100 seconds No, Audio::Timestamp does not currently support that, but it wouldn't be hard to extend it accordingly. After all, it already computes with fractions, so adding some more in isn't such a big deal. The main divergence would be that one would have to represent the "framerate" using a fraction, too, where we use an integer so far. I'll ponder this a bit.
comment:5 by , 15 years ago
The _fileStream to _stream change was only in AviDecoder. It can be changed to _stream in other streams eventually too. Most of the changes are related to the change in the getCurFrame() definition (which is commented on in the patch in video_decoder.h) and the change to using a Surface.
Tucker's frame numbers were changed for the aforementioned getCurFrame() definition change.
We chose double after thinking over a few different solutions for the problem. The AVI has a "scale" and a "rate" field to represent the frame rate. At one point we would have had a scale and rate (with scale defaulted to 1) in VideoDecoder, but the numbers in the file are too high and would overflow very quickly (in about 16 frames) without a 64-bit integer.
comment:6 by , 15 years ago
I don't know about Broken Sword 2, but in Broken Sword 1 (where the cutscenes didn't originally have subtitles), the intro probably can't get away with using 0 and 255 for black and white. When I tried it, the first few lines came out red. (I didn't try this specific patch though. I just changed the current player to use 0 and 255.)
comment:7 by , 15 years ago
mthreepwood, the _fileStream example was just that -- an example. I maintain that the patch is too big for review, and contains many changes that are not described in the patch description; a comment *inside* the patch isn't part of the patch description.
Also, I don't see a comment in the patch explaining why getCurFrame was changed. I can see, looking at the changed comment for this method, *how* it was changed, OK. But this seems to be yet another thing that could be put into a separate patch.
So, please refactor this patch and split it. Using git-svn or quilt, or some similar tools, this shouldn't be too much work, but it'll make it much easier to review this, and eventually apply this patch.
I understand why you used double, but it's still a problem. But it certainly is possible to use integers, by applying some basic math, i.e. canceling fractions. No overflows are necessary, nor 64bit ints. The drawback is some added complexity, which can be mostly avoided by using a suitable class to encapsulate the necessary computations with fractions -- i.e. either an enhanced version of Audio::Timestamp, or a new class which we can design specifically for this; maybe even simply a generic Common::Rational or Common::Fraction class implementing rational numbers.
comment:8 by , 15 years ago
And by the way, I am sorry if I may sound negative -- I am not opposed to this patch, to the contrary, it seems like a step in the right direction. But unfortunately it is kind of an big opaque blob, too, and it'll help tremendously if it was made a bit more transparent -- ideally by breaking it up in either to understand and review steps. This is some work, but I feel that this is quite essential.
comment:9 by , 15 years ago
getCurFrame() was changed because we felt it makes more sense for the "current frame" to be the last one drawn on the screen instead of the next one to be drawn. Therefore, _curFrame is initialized to -1 and becomes 0 after frame 0 is drawn to the screen.
I'm not quite sure how you want the patch split up. Do you want it in incremental steps (ie. change to loading with a stream reference instead of passing the string, then the getCurFrame() change, etc.)?
I understand double is a problem. My point with the example was that the fraction would be 100000/133333 which would produce some rounding when reduced and would overflow in calculations. DrMcCoy is looking into modifying Timestamp for this.
comment:10 by , 15 years ago
Huh? The fraction would be 3/4, so I don't see where the problem is... ?
As for the splitting: Incremental steps, yes. First rename/refactor stuff, then change getCur changes, then the rest. And if there are more parts, great ;)
comment:11 by , 15 years ago
Ugh, splitting this thing now is really a lot of work, since many changes depend on each other. clone said he doesn't have time for this, so I've started making incremental changes to the current interface, but it's just too tedious and frustrating to complete. I have to add crutches left and right to hold up the parts still depending on what's currently there...
What is possible is one patch each for - Introduction of a simple Common::Rational class - Introduction of the general new VideoDecoder and FixedRateVideoDecoder (now using Common::Rational instead of double) interfaces - Adapting FlicDecoder and the tucker engine - Adapting DXADecoder + SmackerDecoder and agos, saga, scumm-he, sword1, sword2 (since DXA was used earlier as a replacement of SMK, sword1/sword2 depends on the interfaces to be the same) - Adapting AviDecoder (and codecs), mohawk and sci (with SeqDecoder and VMDDecoder) - Removing the old VideoDecoder interface
Of course, that's not really incrementally, but rather thematically split and might very well be even worse to review...
comment:12 by , 15 years ago
I haven't asked for the patch to be split up that finely :). I don't see where the problem is in splitting the patch as I described: One patch which renames things (e.g. fileStream -> stream), one that changes the semantics of getCur; and one with "the rest". Maybe the latter then needs to be split further, or not -- I can't say, because right now the first two change sets obscure so much :). If you want to split it further, fine by me, but that's really your choice, and not what I requested :).
By the way, this is a good example why it's best to develop such patches incrementally from the start, using suitable tools (e.g. git, mercurial, quilt, ...) -- much less work than splitting up a monolithic patch later on :)
comment:13 by , 15 years ago
Almost two months later, and finally here is the split up patch. Everything has been tested again (minus SAGA2). Tucker hasn't been tested beyond the intro. The Sword1/Sword2 palette indices situation also remains the same.
The patches are split up like this:
1) getCurFrame() definition change (and endOfVideo() addition) 2) The "rest" of the VideoDecoder API changes 3) Addition of DrMcCoy's Common::Rational class 4) Changing FixedRateVideoDecoder to using Common::Rational instead of floating point math 5) Removal of the old VideoPlayer files
AviDecoder's _fileStream->_stream renaming was reverted, so that patch was not added.
Thanks again to DrMcCoy for assistance and testing.
comment:14 by , 15 years ago
Excellent work! I'll try to review these ASAP. One small thing I wonder about in the patches: You created them with git / git-svn, didn't you? Why then does it contain lots of these:
- * $URL$ - * $Id$ + * $URL: https://scummvm.svn.sourceforge.net/svnroot/scummvm/scummvm/trunk/engines/sci/video/seq_decoder.cpp $ + * $Id: seq_decoder.cpp 47541 2010-01-25 01:39:44Z lordhoto $
? Maybe you copied changes from an SVN checkout? Anyway, this adds a lot of noise to the patches... but is hardly a serious problem ;).
comment:15 by , 15 years ago
Haha. Yes, I did use git-svn to do these, but to shorten the process, I copied some files over from the older code and I forgot that they would have the URL/Id tags filled in still. I would have worried about it more, but it doesn't have an effect when committing via svn. Sorry about that.
comment:16 by , 15 years ago
In class Rational, any particular reason to use the terms _dividend and _divisor instead of the proper terminology numerator and denominator (or just _num and _denom or so)?
Also the code for operator>(int right) etc. is wrong :).
Finally, it would be nice to have the Rational class in a separate commit.
comment:17 by , 15 years ago
"In class Rational, any particular reason to use the terms _dividend and _divisor instead of the proper terminology numerator and denominator (or just _num and _denom or so)?"
No.
:P
"Also the code for operator>(int right) etc. is wrong :)."
Oops, yeah, copy-pasta error :/
comment:18 by , 15 years ago
OK, then please rename the members of class Rational to match the mathematical notation, fix the bugs in operator> etc., and we can already commit that class. Hm, actually, some unit tests would be nice, too (in particular some which catch the current bug in the comparison operators), but we also add those later on.
comment:19 by , 15 years ago
Added a patch with the fixed Rational class and a small unit test for its operators.
comment:20 by , 15 years ago
Rational class now looks pretty good. Please commit it and let's start moving with this patch.
comment:22 by , 15 years ago
Now on the rest of the patches.
Patch3 gets replaced with DrMcCoy's version Patch4 looks OK to commit (after Rational class commit, of course) Patch1: * Instead of ' < frame + 1' I would suggest to write ' <= frame' which is more readable * In decodeNextFrame() put parenthesis in while() conditional statement to increase readability Rest looks OK
Patch 2 (ugh, this one is massive): * Why did you remove curly braces in one line if()s in man places? I would keep them for safety. * Remove those $URL$ changes * in file tucker/sequences.cpp you decrease frame number 126 instead of increasing it. Later in the code you do it correctly with 116 and 706 * in FlicDecoder::decodeNextFrame() it is not clear whether you changed anything or just reindented the region * You kill doxygen comments in video/flic_decoder.h. You should do exactly the opposite, i.e. document more functions :) * In SmackerDecoder::decodeNextFrame() please explain magic numbers in '(_header.flags ? 2 : 1)' in-place * in VideoDecoder::setSystemPalette() please align the palette filling code for better readability
Patch 5 is OK :)
In short, please commit patches 1, DrMcCoy's & 4. Patch 2 needs more work. Looking forward to updated patch. Preferably split it into two, i.e. video decoder changes and engine changes.
comment:23 by , 15 years ago
The Common::Rational patches from DrMcCoy have been committed. I also committed patch 1 with the changes you mentioned (I assumed you meant AviDecoder::decodeNextFrame(), and I replaced part with an endOfVideo() call instead). Patch 4 cannot be committed as it requires Patch 2 first.
In regards to Patch 2:
> * Why did you remove curly braces in one line if()s in man places? I > would keep them for safety. I removed them because I hate extra curly braces in there. I don't think they provide much safety, if any. I can try to add some back if you want.
I'll have the other things you mentioned in a new patch (split between engine code and video code) and ready to go soon.
by , 15 years ago
| Attachment: | vid_dec_graphics.diff added |
|---|
Patch (graphics directory) against r49069
comment:24 by , 15 years ago
Updated patch 2 with changes you mentioned (split between engines/ and graphics/). How many doxygen comments do you want in flic_decoder.h? I'm not sure we need to have comments on every function when the same documentation is in video_decoder.h.
Patch 4 (the double->Common::Rational patch) has been merged into this. I figured there wasn't a point in having the double code at all in the patch anymore.
comment:25 by , 15 years ago
patch for videos looks almost OK. Only thing is that I recommend to add parenthesis in conditional expression in VideoDecoder::endOfVideo().
Patch for engines: * I am not sure about _frameCounter change in engines/tucker/sequences.cpp. Did you test it? To me it seems that the counter should stay at 80. * You are defining SCREEN_WIDTH and SCREEN_HEIGHT constants in sci/video/seq_decoder.cpp, but still are using hardcoded constants in getWidth() in seq_decoder.h. I recommend to move the defines up, add prefix to them, i.e. SEQ_SCREEN_WIDTH and use them everywhere. * You made palette array size x3 instead of x4 in sci/video/seq_decoder.cpp in loadFIle(). Are you sure about that? * in scumm/he/animation_he.cpp Common::String constructor in loadFile() call is redundant
Please fix above issues, commit, and e-mail to scummvm-devel with call of the engine authors to review/retest. I envision regressions.
comment:26 by , 15 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:27 by , 15 years ago
Committed with the changes. I sent an e-mail off to -devel, as you requested as well. It's finally done! :)
comment:28 by , 6 years ago
| Component: | → Video |
|---|
Patch against r48158