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

hwdec: support videotoolbox with libplacebo #12776

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

rcombs
Copy link
Contributor

@rcombs rcombs commented Oct 30, 2023

Relevant now that #7482 is merged. Also removes some code duplication between the macOS and iOS paths.

@github-actions
Copy link

github-actions bot commented Oct 30, 2023

Download the artifacts for this pull request:

Windows

meson.build Outdated Show resolved Hide resolved
Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if hwdec_vt_pl is supposed to work on ios or not. I'm guessing no and if so, you would need to change the checks a bit so that doesn't slip through.

video/out/gpu/hwdec.c Outdated Show resolved Hide resolved
video/out/hwdec/hwdec_vt.c Outdated Show resolved Hide resolved
video/out/hwdec/hwdec_vt.h Outdated Show resolved Hide resolved
video/out/hwdec/hwdec_vt_pl.m Outdated Show resolved Hide resolved
video/out/hwdec/hwdec_vt.c Outdated Show resolved Hide resolved
@m154k1
Copy link
Contributor

m154k1 commented Nov 5, 2023

I tried to address all comments except objective-c one (the check seems to be false for me?):
https://github.com/m154k1/mpv/commit/20ed4d068794babde3976cef879d2a33ae0e9d1a

@Dudemanguy, @rcombs, what do you think?

@Dudemanguy
Copy link
Member

That was what I had in mind except I'm not sure if features['gl-cocoa'] is actually needed here.

except objective-c one (the check seems to be false for me?)

I don't think it's needed at all? The struct members are already guarded by HAVE_VULKAN && COREVIDEO_SUPPORTS_METAL (or HAVE_VIDEOTOOLBOX_PL in your changes).

@m154k1
Copy link
Contributor

m154k1 commented Nov 5, 2023

I'm not sure if features['gl-cocoa'] is actually needed here.

Hmm, we need at least cocoa here as macvk is cocoa-only. I tried to build mpv with gl=disabled but it crashes with symbol not found in flat namespace '_ra_hwdec_videotoolbox' so I guess it's better to have the gl.

I don't think it's needed at all?

Do you mean to keep the c or objective-c code?
This code works fine:

#if HAVE_VIDEOTOOLBOX_PL
    void* mtl_texture_cache;
    void* mtl_planes[MP_MAX_PLANES];
#endif

but I can't build this one:

#if HAVE_VIDEOTOOLBOX_PL
    CVMetalTextureCacheRef mtl_texture_cache;
    CVMetalTextureRef mtl_planes[MP_MAX_PLANES];
#endif
error: unknown type name 'CVMetalTextureCacheRef'
error: unknown type name 'CVMetalTextureRef'

@Dudemanguy
Copy link
Member

Hmm, we need at least cocoa here as macvk is cocoa-only. I tried to build mpv with gl=disabled but it crashes with symbol not found in flat namespace '_ra_hwdec_videotoolbox' so I guess it's better to have the gl.

Sure cocoa sounds fine. No ideal about that particular error, but gl-cocoa only includes a couple of opengl-related files, so I wouldn't think whatever symbol is missing needs to specifically live only there.

but I can't build this one:

I guess you need to include #include <Metal/Metal.h> there?

@orion1vi
Copy link
Contributor

orion1vi commented Nov 5, 2023

Rename hwdec_vt.c -> hwdec_vt.m ; hwdec_mac_gl.c -> hwdec_mac_gl.m and it should work.

Hmm, we need at least cocoa here as macvk is cocoa-only. I tried to build mpv with gl=disabled but it crashes with symbol not found in flat namespace '_ra_hwdec_videotoolbox' so I guess it's better to have the gl.

if features['videotoolbox-gl']
    sources += files('video/out/hwdec/hwdec_vt.m',
                     'video/out/hwdec/hwdec_mac_gl.m')
endif

if features['videotoolbox-pl']
    sources += files('video/out/hwdec/hwdec_vt.m',
                     'video/out/hwdec/hwdec_vt_pl.m')
endif

Should do it.
Also I don't think COREVIDEO_SUPPORTS_METAL is necessary with recent minimum version bump and for videotoolbox-gl features['gl'] or features['ios-gl'] instead of features['gl-cocoa'] or features['ios-gl'] should be better for libmpv user.

Perhaps it would also be nicer to define it in the same way as videotoolbox_gl.

videotoolbox_pl = get_option('videotoolbox-pl').require(
    features['vulkan'],
    error_message: 'vulkan could not be found!',
)
features += {'videotoolbox-pl': videotoolbox_pl.allowed()}

@m154k1
Copy link
Contributor

m154k1 commented Nov 5, 2023

Sure cocoa sounds fine. No ideal about that particular error, but gl-cocoa only includes a couple of opengl-related files, so I wouldn't think whatever symbol is missing needs to specifically live only there.

Okay, let's use cocoa then.

Rename hwdec_vt.c -> hwdec_vt.m ; hwdec_mac_gl.c -> hwdec_mac_gl.m and it should work.

That works, thank you!

Also I don't think COREVIDEO_SUPPORTS_METAL is necessary with recent minimum version bump and for videotoolbox-gl features['gl'] or features['ios-gl'] instead of features['gl-cocoa'] or features['ios-gl'] should be better for libmpv user.

Not sure about this. Should be a separate PR probably.

Perhaps it would also be nicer to define it in the same way as videotoolbox_gl.

I agree, added too.

Updated patch: https://github.com/m154k1/mpv/commit/b5c5ec92e9407c5490345f81c18c56d4fcbb7ba9

@orion1vi
Copy link
Contributor

orion1vi commented Nov 5, 2023

Does it not work without corevideo_metal?

@m154k1
Copy link
Contributor

m154k1 commented Nov 5, 2023

Well, it works. I just wasn't sure if it's available across different devices/OSes. But I guess it should be available everywhere since 10.15+ is required.

Added here: https://github.com/m154k1/mpv/commit/4e03db3ae7bdba3ca41762b4677b6fea38c53cc2

@rcombs
Copy link
Contributor Author

rcombs commented Nov 6, 2023

Re-pushed. There's no support for vulkan on iOS currently (as there's no support for vulkan embedding anywhere yet, nor wid embedding on iOS), but this should work fine there once either of those things is added.

meson.build Show resolved Hide resolved
Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't speak for any of the mac specifics but LGTM from just a general overview.

meson.build Outdated Show resolved Hide resolved
@Akemi
Copy link
Member

Akemi commented Nov 16, 2023

the technicalities of hwdec are not really my expertise, though the code looks fine and i checked that everything works like before (at least on macOS, can't check iOS). i also checked that the none-copy vt works as expected now with the macvk backend.

LGTM and works as expected. think it would be good to get this merged before the next release, so we get some testing in with the new backend. any problems that might pop up can be fixed afterwards/with the release after the upcoming one.

@Akemi Akemi added this to the Release checklist v0.37.0 milestone Nov 16, 2023
@rcombs rcombs merged commit ca45b71 into mpv-player:master Nov 16, 2023
13 checks passed
@rcombs rcombs deleted the mac-placebo-hwdec branch November 16, 2023 17:48
@byMohamedali
Copy link

Hi @rcombs @Akemi , thanks for the feature, any indication on how to use it for iOS and macOS. so far what i tried, but i'm not sure what to pass on for wid option

 chkErr(mpv_set_property_string(mpv, "vo", "gpu"))
 chkErr(mpv_set_property_string(mpv, "gpu", "vulkan"))
 chkErr(mpv_set_option_string(mpv, "hwdec", "videotoolbox-copy"))
 chkErr(mpv_set_option_string(mpv, "gpu-hwdec-interop", "videotoolbox-copy"))
 chkErr(mpv_set_option(mpv, "wid", MPV_FORMAT_INT64, &metalView.displayLayer))

Thanks

@Akemi
Copy link
Member

Akemi commented Nov 26, 2023

wid embedding is atm not supported. libmpv usage was only recently fixed by me 3f2bc2e. so make sure to use a recent enough version.

@byMohamedali
Copy link

Thanks for your answer, So then how should we use it ? is there any sample or maybe can you provide quick guide on what should we do

@andrefmsilva
Copy link

@byMohamedali did you manage to make it work in iOS or tvOS?
I've tried with #7857 that was support to send the Metal layer with --wid but i can't make it work. The implementation in that PR is basically a copy of the android version with adjustments to use metal.

maybe @Akemi can help, but right now i'm getting this assert:
haasn/libplacebo#229 (comment)

@andrefmsilva
Copy link

andrefmsilva commented Dec 11, 2023

I did a few more tests and it works in iOS!!
the problem is only with tvOS.

@byMohamedali
Copy link

hey @andrefmsilva i was not able to make it work, if you can share the implementation it would be nice :)
i will try as well on tvOS and let you know, you've been able to make it work with metal ?

@andrefmsilva
Copy link

andrefmsilva commented Dec 11, 2023

Yes, for me it's working in iOS (using the vo in #7857). You just need to use the code in that PR and make meson compile the file.

mpv_set_option(mpv, "wid", MPV_FORMAT_INT64, &playerLayer)
        
mpv_set_property_string(mpv, "vo", "gpu")
mpv_set_property_string(mpv, "gpu-api", "vulkan")
mpv_set_property_string(mpv, "gpu-context", "moltenvk")
mpv_set_property_string(mpv, "hwdec", "videotoolbox")

@byMohamedali
Copy link

Thanks i will test and let you know for tvOS

@byMohamedali
Copy link

By the way have you test videos with subtitles ? is it working with this implementation ?

@andrefmsilva
Copy link

No, I only did some basic tests in a sample app that I have.

@karelrooted
Copy link
Contributor

karelrooted commented Dec 12, 2023

By the way have you test videos with subtitles ? is it working with this implementation ?

subtitle works in this implementation (this is a patch that works on latest git master)

with the latest code, some of the old patch code may not be necessary

test on iPad, gpu-next works too

final class MPVViewController: UIViewController {
    override func viewDidLoad() {
        super.loadView()
        let client = MPVClient()
        client.create(frame: view.frame)
        view.layer.addSublayer(client.metalLayer)
        super.viewDidLoad()
    }
}

final class MPVClient: ObservableObject {
    var metalLayer = CAMetalLayer()
    func create(frame: CGRect? = nil) {
        metalLayer.frame = frame!
        metalLayer.contentsScale = UIScreen.main.nativeScale
        metalLayer.framebufferOnly = true
        mpv_set_option(mpv, "wid", MPV_FORMAT_INT64, &metalLayer)
        mpv_set_property_string(mpv, "vo", "gpu-next")
        mpv_set_property_string(mpv, "gpu-api", "vulkan")
        mpv_set_property_string(mpv, "hwdec", "videotoolbox")
    }
}

@andrefmsilva
Copy link

Thanks @karelrooted , that is basically what i did to get it working. The image has quality? i only did some quick tests but the image was low quality. Tried to use another profile in MPV but it didn't work.

Is it working for you in tvOS or do you get the same crash?

@byMohamedali
Copy link

@andrefmsilva for the low quality you need to set the drawableSize of the metalLayer to the size of the video you're reading.

so for example for a 1920x1080

metalLayer.drawableSize = CGSize(width: 1920, height: 1080)

I tried in tvOS, i don't have a crash but a bluescreen is anyone of you has the same issue ?

@karelrooted
Copy link
Contributor

karelrooted commented Dec 12, 2023

The image has quality? i only did some quick tests but the image was low quality. Tried to use another profile in MPV but it didn't work.

did you set contentsScale ?

metalLayer.contentsScale = UIScreen.main.nativeScale

without setting this property, 4k video works fine, 1080p video's quality will seems low

Is it working for you in tvOS or do you get the same crash?

tvOS doesn't work too(compile with all latest git master), crash triggered when using vo=libmpv && vf=libplacebo

when using vo=gpu/gpu-next , no crash is triggered, but mpv play video file with audio but no video , fulllog

@andrefmsilva
Copy link

andrefmsilva commented Dec 12, 2023

Thanks! Please keep me updated if you find new things about this. Maybe @karelrooted can create a PR with that patch so we can get "oficial" support for iOS

The erros in tvOS is this one?
haasn/libplacebo#229

@karelrooted
Copy link
Contributor

karelrooted commented Dec 12, 2023

Thanks! Please keep me updated if you find new things about this. Maybe @karelrooted can create a PR with that patch so we can get "oficial" support for iOS

the original PR is closed, mpv team must have other plan for this

The erros in tvOS is this one? haasn/libplacebo#229

error is similar, but not the same

iOS and MacOS has another interesting bug when under vo=libmpv && vf=libplacebo , crash under Xcode debug mode, but works fine when run app normally without Xcode debug (it seems gpu-context=moltenvk will trigger the same debug mode bug too, but only triggerd occasionally , not like vf=libplacebo 's will surely triggered ,usually within 60 second)

Update: tvOS is fixed with MoltenVK latest git

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

Successfully merging this pull request may close these issues.

9 participants