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

IMAGE: Fix cinepak conversion to 8bpp for some Myst (1994) videos #4157

Closed
wants to merge 1 commit into from

Conversation

antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Aug 4, 2022

This addresses bug ticket #13479

The fix should not have side-effects, but someone more familiar with the code for the cinepak decoder should review this.

I can see some black artifacts at the start of the videos so maybe something else is needed as well, or a different approach altogether. Relevant short discord discussion starts here: https://discord.com/channels/581224060529148060/581224061091446795/975812864721956924

This addresses bug ticket #13479

The fix should not have side-effects, but someone more familiar with the code for the cinepak decoder should review this.
Relevant short discord discussion starts here: https://discord.com/channels/581224060529148060/581224061091446795/975812864721956924
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Aug 4, 2022

Is this the same bug that I'm fixing in QuickTimeDecoder? #4156

If so, then it's a regression I caused in November when I fixed some QuickTimeDecoder bugs but broke a rule I didn't know about where decoding a frame isn't allowed when loading a stream, because that in turn breaks VideoDecoder::setDitheringPalette

From the comments on VideoDecoder::setDitheringPalette:

* This should be called after loadStream(), but before a decodeNextFrame()
* call. This is enforced.

I don't have Myst but I would start by testing the QuickTimeDecoder fix and see if that takes care of this.

@antoniou79
Copy link
Contributor Author

@antoniou79 antoniou79 commented Aug 4, 2022

Is this the same bug that I'm fixing in QuickTimeDecoder? #4156

If so, then it's a regression I caused in November when I fixed some QuickTimeDecoder bugs but broke a rule I didn't know about where decoding a frame isn't allowed when loading a stream, because that in turn breaks VideoDecoder::setDitheringPalette

From the comments on VideoDecoder::setDitheringPalette:

* This should be called after loadStream(), but before a decodeNextFrame()
* call. This is enforced.

I don't have Myst but I would start by testing the QuickTimeDecoder fix and see if that takes care of this.

You're correct! Your code works, and it works better -- no black artifacts at the start of video. So this PR can be closed.

I actually was reminded that this Myst bug and proposed patch were pending for a while, because I read your PR today, but it didn't seem directly related at least not by looking at the files. But, I did intend to check it out anywyay, then other life things happened, so thanks for the extra nudge!

@antoniou79 antoniou79 closed this Aug 4, 2022
@sluicebox
Copy link
Member

@sluicebox sluicebox commented Aug 4, 2022

Thanks for testing that; I've updated my commit message to include Myst and the bug ticket

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