Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VIDEO: Remove logically dead code #460

Merged
merged 1 commit into from
Apr 18, 2014
Merged

Conversation

klusark
Copy link
Contributor

@klusark klusark commented Apr 10, 2014

It is impossible for this code to run as both newWidth and newheight will never be 0

@digitall
Copy link
Member

@klusark : While you are correct, I think that the code is probably intended to do something different and so should be fixed, rather than removed.

I wonder if someone got their De Morgan's law confused and managed to get a && rather than an intended || in the first statement:
https://en.wikipedia.org/wiki/De_Morgan%27s_laws#Engineering

ie. line 179 should be if ((newWidth != 0) || (newHeight != 0)) {

@digitall
Copy link
Member

Doing a git blame, this code was all added at the same time by commit 11cbdd0 (svn-id: r49079) by @clone2727 .

However, as the commit message indicates, he was just committing a patch:
"Committing the rest of the VideoDecoder Rewrite from patch #2963496."

@digitall
Copy link
Member

Ah, this in the new patch tracker is now patch #1246:
http://sourceforge.net/p/scummvm/patches/1246/

@clone2727 clone2727 closed this Apr 10, 2014
@clone2727 clone2727 reopened this Apr 10, 2014
@clone2727
Copy link
Contributor

Indeed, should be "||" instead of "&&".

But don't blame DeMorgan! (Did you really have to paste a Wikipedia link?!)

@klusark
Copy link
Contributor Author

klusark commented Apr 10, 2014

I figured I'd just submit this instead of figuring out what it should really do as I don't have any games that use this system, so I can't test it anyway. Want me to resubmit with || instead? I just don't know what issues that could potentially cause though.

@clone2727
Copy link
Contributor

Submit and I'll merge it.

Likely nothing uses this in master. I don't think Tucker uses it. It might be used in kom, but @salty-horse may have no samples either. Video resizing sucks in general, and I doubt anything we might needed for uses it.

The code currenly has 4 logically dead lines. Instead of requiring
both newWidth and newHeight to be non zero, just make sure one of
them is non zero and set the other one to the current size.
@salty-horse
Copy link
Member

I don't think kom "uses" it. Is it supposed to handle buggy flic files?
This is the original patch discussion: http://sourceforge.net/p/scummvm/patches/1246/

BTW, the commit message of the fix has a typo: "currenly"

@digitall
Copy link
Member

@salty-horse : Thanks for the link, but I did link that already. My interest in this was purely that I was hoping to get the other out-of-tree fixes to the FLIC decoder in the pink engine merged at some point, so that it doesn't bitrot:
adamrimon@6d81b39

As this is a one line fix of a very obvious minor oversight, it would be nice to get this merged fairly soon... even if it gets cherry-picked and fixed.

@salty-horse
Copy link
Member

Sorry, I skimmed the conversation and misse it. I checked, and all the FLIC files in tucker and kom have 0 for their frame width/height. This means that they never resize the video.

I don't have Passport to Peril at the moment to check if it requires that functionality.

clone2727 added a commit that referenced this pull request Apr 18, 2014
VIDEO: Use || instead of the currently incorrect &&
@clone2727 clone2727 merged commit d4756a9 into scummvm:master Apr 18, 2014
@clone2727
Copy link
Contributor

Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants