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

FFV1 fix #7874

Merged
merged 2 commits into from Mar 15, 2019

Conversation

5 participants
@vadosnaprimer
Copy link
Contributor

vadosnaprimer commented Mar 10, 2019

Change pixel format from BGRA to BGR0

For this kind of footage carrying alpha information makes no sense, and it additionally complicates things by hugely damaging compatibility of the resulting video. After this change alone the video becomes compatible with VfW/WinAPI and tools that rely on it (avisynth, virtualdub).

Fixes https://bugs.dolphin-emu.org/issues/11141 and https://bugs.dolphin-emu.org/issues/10193

Note that internally pixel format seems to be RGBA or BGR24, but then it's converted to whatever is set for ffmpeg, and I'm only changing the latter.

Decrease gop size (keyint)

This makes seeking a lot smoother (especially at high resolutions), while only adding less than 1% of filesize overhead with this codec.

@stenzek

This comment has been minimized.

Copy link
Contributor

stenzek commented Mar 11, 2019

Could you please rebase this on the latest master? Looks like our FifoCI buildbots are failing to build because it's missing the ICE fix.

@stenzek
Copy link
Contributor

stenzek left a comment

LGTM. The alpha channel should be set to 1 anyway for all frames, so I can't see this hurting anything.

FifoCI says it's fine (FFV1 is used for frame dumping there IIRC).

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Mar 11, 2019

You'll need to rebase the branch so that there isn't a merge commit in it.

@vadosnaprimer

This comment has been minimized.

Copy link
Contributor Author

vadosnaprimer commented Mar 11, 2019

@JosJuice will it remain there if the PR is squashed? I don't know what went wrong on my end. Commits got duplicated for whatever reason as well, so I wonder if squashing helps, since the diff is just 2 numbers.

@BhaaLseN

This comment has been minimized.

Copy link
Member

BhaaLseN commented Mar 11, 2019

It will not remain in there if you do it properly. They likely got duplicated because you pulled from your own remote before pushing.

You can try one of the following:

  1. Rebase on latest master and assume it will-just-work™ (replace origin with our remote name):
git fetch origin
git rebase origin/master ffv1_fix
  1. Reset on latest master and redo your commits (unlikely to leave commits behind you don't want):
git fetch origin
git checkout ffv1_fix
git reset --soft origin/master
git commit (or git gui, or your preferred method of committing)

Once this is done and your history looks ok, you want git push -f (or git push -f yourRemote ffv1_fix)

vadosnaprimer added some commits Mar 10, 2019

change pixel format from BGRA to BGR0
for this kind of footage carrying alpha information makes no sense, and it additionally complicates things by hugely damaging compatibility of the resulting video. after this change alone the video becomes compatible with VfW/WinAPI and tools that rely on it (avisynth, virtualdub).

fixes https://bugs.dolphin-emu.org/issues/11141 and https://bugs.dolphin-emu.org/issues/10193
decrease gop size (keyint)
this makes seeking a lot smoother (especially at high resolutions), while only adding less than 1% of filesize with this codec.

@vadosnaprimer vadosnaprimer force-pushed the vadosnaprimer:ffv1_fix branch from 7103d68 to 7fd9404 Mar 11, 2019

@vadosnaprimer

This comment has been minimized.

Copy link
Contributor Author

vadosnaprimer commented Mar 13, 2019

Is it okay now? One of the builds failed, but the reason doesn't seem to be relevant.

@JosJuice

This comment has been minimized.

Copy link
Contributor

JosJuice commented Mar 13, 2019

Yeah, it looks fine. The macOS buildbot is just broken in general right now.

@RisingFog, do you want to take a look at this PR?

@RisingFog

This comment has been minimized.

Copy link
Member

RisingFog commented Mar 15, 2019

Looks good to me.

@JosJuice JosJuice merged commit c712164 into dolphin-emu:master Mar 15, 2019

9 of 10 checks passed

pr-osx-x64 Build failed on builder pr-osx-x64
Details
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.