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

libplacebo v6 updates #11952

Merged
merged 9 commits into from Aug 18, 2023
Merged

libplacebo v6 updates #11952

merged 9 commits into from Aug 18, 2023

Conversation

haasn
Copy link
Member

@haasn haasn commented Jul 17, 2023

Also includes the new HDR contrast recovery option. I included it by default in the gpu-hq profile.

Question: Should we "downgrade" HDR peak detection from on-by-default to on-in-gpu-hq as well? It's really punishing for slower devices, even after libplacebo v6's optimizations.

@haasn
Copy link
Member Author

haasn commented Jul 17, 2023

This is probably blocked by VLC 3 being incompatible with libplacebo v6, as well as ffmpeg 6.0 (6.1 will have a fix for it)

@haasn
Copy link
Member Author

haasn commented Jul 17, 2023

Well, this breaks CI, which expects mpv to build with whatever versions of libplacebo are available on the CI image.

Maybe we should hold off on requiring v6 for the time being? Thoughts?

@christoph-heinrich
Copy link
Contributor

christoph-heinrich commented Jul 17, 2023

Without v6 --hdr-contrast-recovery and --hdr-contrast-smoothness aren't available which is a shame.
They could be gated with defines, so that they are available to those running newer versions. In that case the documentation would have to mention that for those v6 is required.

Downgrading HDR peak detection seems reasonable, but I don't know how slow we're talking of.
It works fine on my ADM R9 380 (with plenty of headroom to spare), which is slow by today's standards, but I haven't tried it on any iGPUs.

@m154k1
Copy link
Contributor

m154k1 commented Jul 17, 2023

This is probably blocked by VLC 3 being incompatible with libplacebo v6, as well as ffmpeg 6.0 (6.1 will have a fix for it)

Is it really a blocker for mpv? Distributions can provide libplacebo5 and libplacebo6 packages.

Well, this breaks CI, which expects mpv to build with whatever versions of libplacebo are available on the CI image.

CI can be fixed by introducing a meson subproject. However this requires disabling waf.

Should this be removed too?

option('libplacebo-next', type: 'feature', value: 'auto', description: 'gpu-next video output')

@sfan5
Copy link
Member

sfan5 commented Jul 17, 2023

Maybe we should hold off on requiring v6 for the time being? Thoughts?

I suggest waiting for the release at least, so that distributions can support vulkan for vo_gpu.
gpu_next is out of reach anyway as the only placebo release that matches the requirement is v6.

@llyyr
Copy link
Contributor

llyyr commented Jul 17, 2023

I think distributions can ship multiple versions of libplacebo, I don't know how long you can keep waiting for VLC since both mpv and ffmpeg 6.1 do want latest libplacebo.

But if we aren't bumping libplacebo to v6, then I think it makes sense to also put #11875 behind preprocessors, since this one change is the only reason why libplacebo v5 can't have libplacebo-next

@philipl
Copy link
Member

philipl commented Jul 18, 2023

Also includes the new HDR contrast recovery option. I included it by default in the gpu-hq profile.

Question: Should we "downgrade" HDR peak detection from on-by-default to on-in-gpu-hq as well? It's really punishing for slower devices, even after libplacebo v6's optimizations.

With your latest optimisations, it became much more usable (4k60 worked on a UHD770 for the first time) but gpu-hq is too heavy for the UHD770 and not quite usable on the Iris Xe class iGPU. And given how visually distorted things look without peak detection, I would keep it on my default. It now works on more low end hardware, and I think it's a better trade-off to make people turn it off for being slow than for other people to work out how to turn it on for hardware where gpu-hq is too much.

ie: I think that going into the future, hardware that is too slow for peak detection is going away while hardware that can do peak detection but cannot do gpu-hq will be the more common low end combination.

@philipl
Copy link
Member

philipl commented Jul 18, 2023

You can simplify this too:

image

(github doesn't let you comment on lines that are "too far away" from the changes in the diff)

@philipl
Copy link
Member

philipl commented Jul 19, 2023

intel's iGPUs have ASIC video processors, so if everything else is offloaded to ASIC (scaling, format conversion, etc.), maybe peak detection will become feasible?

Theoretically? Maybe. It would require extensive and invasive changes to the libplacebo pipeline to actually do properly - you can't achieve identical results using vaapi/qsv filters as-is, as steps will happen in the wrong order. You'd also be giving up the superior algorithms in libplacebo for whatever the hardware gives you. You can take that to the logical conclusion and just use the hardware tone-mapping and offload everything and be happy with whatever that gives you. Of course, the Intel iGPUs with hardware tone mapping are also the ones that can do peak detection anyway, so...

@llyyr
Copy link
Contributor

llyyr commented Jul 19, 2023

There's not much point in that, the UHD 770 is good enough to do 4k60 peak detection on a DDR5 system, but lags when I underclock the same memory to DDR4 speeds. The bottleneck here is purely due to the igpu not having fast enough dedicated memory. Even if you used the hardware better you can't make the memory bus go faster

@haasn haasn marked this pull request as draft July 19, 2023 13:47
@kasper93
Copy link
Contributor

This is probably blocked by VLC 3 being incompatible with libplacebo v6, as well as ffmpeg 6.0 (6.1 will have a fix for it)

(also in response to #11991, but I don't want to split discussion)

When is the plan to release stable mpv? Maybe we could wait until dust settles? And release it once all new fancy features are available out of the box, at least in Arch.

Currently neither new HDR Tone Mapping, nor Vulkan decoding would be available. With stable versions of available packages.

Of course this works only if it would be relatively short time frame. I'm not suggesting blocking release indefinitely.

@sfan5
Copy link
Member

sfan5 commented Jul 23, 2023

When is the plan to release stable mpv?

I was thinking today or early next week.
Worth noting that the last major mpv release is more than 6 months old, so I don't think it's useful to wait any further.
If we want to bring the new stuff enabled by libplacebo v6 to end users early we can always release 0.37.0 quicker.

@haasn
Copy link
Member Author

haasn commented Jul 23, 2023

Rebased and re-introduced the pl_dispatch_info_move commit.

@haasn
Copy link
Member Author

haasn commented Jul 23, 2023

The CI issues still need to be resolved, we can't merge this until libplacebo6 exists on the appropriate platforms, unless we can somehow vendor it in as a meson subproject / git submodule.

@nekopsykose
Copy link
Contributor

could probably add libplacebo into https://github.com/mesonbuild/wrapdb and then the wrapdb fallback would easily work. since waf is dropped that makes it much easier to maintain

nekopsykose added a commit to nekopsykose/mpv that referenced this pull request Jul 23, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
@nekopsykose
Copy link
Contributor

nekopsykose commented Jul 23, 2023

alternatively just a wrapfile, like in the referenced mr

nekopsykose added a commit to nekopsykose/mpv that referenced this pull request Jul 23, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
@kasper93
Copy link
Contributor

The CI issues still need to be resolved, we can't merge this until libplacebo6 exists on the appropriate platforms, unless we can somehow vendor it in as a meson subproject / git submodule.

Here is my view on this topic. While it is important to test against latest master of libplacebo, which ironically only mingw build do. It is also important that mpv is buildable on some common or maybe not so common platforms. And compatible with latest stable packages they provide.

If the idea, moving forward, is that mpv supports only libplacebo6 it is fine, gpu-next and vulkan support will not be available on platforms that does not package libplacebo6. It is not a CI issue, it is just something that CI shows.

I understand libplacebo is close to mpv, but I don't like the idea of coupling them together with wraps. Otherwise why not do the same for ffmpeg or libass? Package maintainers will not use wrap/submodules anyway, so I don't see added value of them. If you want to use newer libplacebo just build it yourself.

Maybe we should just wait a bit until libplacebo6 is available... anywhere. As I see we are locked on vlc3. We can push maintainers to package libplacebo6 as separate package, I can do that for msys2.

@llyyr
Copy link
Contributor

llyyr commented Jul 23, 2023

Packaging an older libplacebo version for VLC is the way to go here, I think. I've already sent a request for TW, it's up to others to push their distro to do it as well. This shouldn't be hard considering there are already distros that package multiple versions of fast moving libraries, like wlroots.

The Linux CI at the very least will work once TW updates their libplacebo, since we use TW for the CI.

@nekopsykose
Copy link
Contributor

nekopsykose commented Jul 24, 2023

speaking as a distro maintainer (though one that's a little biased, because i maintain this stack there): having a different-version-but-the-same-library with a distinct filename/soname isn't extremely rare (not preferred, but not insurmountable). the only risk that occurs is:

  • 'development' packages conflict with eachother because of include-dir/non-suffixed-.so location (this is irrelevant really)
  • loading multiple versions of the same library into the same process doesn't work without being broken (assuming they conflict in symbols, which is pretty much always the case, and here). in this case, ffmpeg can also link to libplacebo- so if a distro builds ffmpeg with libplacebo support, it must use the same one as mpv (assuming mpv also uses that same ffmpeg). in this case, the ffmpeg for vlc3 (4) would be distinct anyway (assuming a distro had two ffmpeg packages), and the latest release of ffmpeg cannot use libplacebo 6 without applying two patches. but this is not really much work to fix... (also most distros have more than one version of ffmpeg already, as an example, because of the large 4->5 break)

overall this is a chicken-and-egg problem. nobody will update libplacebo if they have no reason to duplicate it, since it's preferred to have one library. if you don't want to require 'v6 only' (as opposed to the ifdefs allowing both), then every distro will probably just keep v5-only for as long as vlc needs it (theoretically vlc3 can't even use v5 without a patch..). you can very easily give them a gentle nudge to use the newer one by making it the only compatible one- and this would only happen on master, as 0.36 was released without these changes.

the only actual negative effect that would probably happen, is people building mpv themselves while relying on their own distro packages for the deps (as opposed to using e.g. mpv-build). those might suddenly show up wanting support :)

then every distro will probably just keep v5-only for as long as vlc needs it

another solution- i'm not aware of any libplacebo users in distro packages aside from mpv, vlc, and ffmpeg. the former is this, the latter is just two small patches, and the middle one is the odd one out. if someone ported the 3.x vlc branch to v6 (i tried once actually, but that is not a small change :D), then everyone could just use that patch and then that solves the whole issue here too.

@llyyr
Copy link
Contributor

llyyr commented Jul 24, 2023

loading multiple versions of the same library into the same process doesn't work without being broken (assuming they conflict in symbols, which is pretty much always the case, and here). in this case, ffmpeg can also link to libplacebo- so if a distro builds ffmpeg with libplacebo support, it must use the same one as mpv (assuming mpv also uses that same ffmpeg).

This is a good point actually, I hadn't thought about it but fortunately this shouldn't be a problem considering VLC3 already requires ffmpeg-4 to build which doesn't even have libplacebo while mpv can build with ffmpeg-6. Nearly all distros already carry both ffmpeg-4 and ffmpeg-6 already.

latest release of ffmpeg cannot use libplacebo 6 without applying two patches

ffmpeg 6.1 should be very soon so those patches won't be needed anymore.

if someone ported the 3.x vlc branch to v6

This would be very not trivial, you may as well just build off the master branch for VLC 4 at that point, or build VLC 3 without libplacebo

@kasper93
Copy link
Contributor

kasper93 commented Jul 24, 2023

ffmpeg [...] don't see major API changes very often, and new features (mainly various decoders?) will not have a major impact on the user experience

I don't agree with this assessment. Right now big feature that some people want to try is Vulkan decoding. Also ffmpeg is quite a bigger library, used ubiquitously. There is much more effort put to keep API/ABI compatible and rules when the API/ABI can be broken and so on. And in fact ffmpeg4.4 is still packaged for projects that weren't updated to newer version and likely will never be.

This is a good point actually, I hadn't thought about it but fortunately this shouldn't be a problem considering VLC3 already requires ffmpeg-4 to build which doesn't even have libplacebo while mpv can build with ffmpeg-6. Nearly all distros already carry both ffmpeg-4 and ffmpeg-6 already.

vlc3 is indeed build with ffmpeg4.4 on Arch, but I just taken a look at msys2 and they seem to build it with ffmpeg6.

haasn and others added 8 commits August 18, 2023 15:38
To remove a bunch of #ifdef templating.
LIBPLACEBO_NEXT is now implied by libplacebo being available
Now implied by the minimum libplacebo version.
v6.292 implied by minimum dependency.
Instead copy the data on-demand when VOCTRL_PERFORMANCE_DATA is
requested.
@haasn
Copy link
Member Author

haasn commented Aug 18, 2023

Judging by discussion on IRC it seems disabling CI on MSYS2 (to unblock this PR) is not a big deal, because we already test building libplacebo on mingw.

With that out of the way, any last objections to this PR?

Instead of `quarterly`, to get access to recent packages.
@haasn haasn merged commit ac4c88b into mpv-player:master Aug 18, 2023
13 checks passed
@haasn haasn deleted the pl6 branch August 18, 2023 14:40
nekopsykose added a commit to nekopsykose/mpv that referenced this pull request Sep 17, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 19, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 20, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 20, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 20, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 21, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 21, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Sep 26, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Oct 7, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
llyyr pushed a commit to llyyr/mpv that referenced this pull request Oct 17, 2023
most distros don't have a v6 yet, so this is needed for
mpv-player#11952
haasn pushed a commit that referenced this pull request Oct 23, 2023
most distros don't have a v6 yet, so this is needed for
#11952
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.

None yet

9 participants