ValveSoftware / wine

Wine with a bit of extra spice
Other
1.3k stars 243 forks source link

mfplat: support http/https/rtsp schemes #142

Closed yshui closed 2 years ago

yshui commented 2 years ago

This leverages gstreamer's uridecodebin to handle http/https/rtsp URLs. This fixes games that try to play video from a URL, such as ValveSoftware/Proton#1199, ValveSoftware/Proton#5195, etc.

This patch isn't a proper implementation of handlers of these schemes on top of the Windows API, but I think this is a quick and simple stop gap solution to bring a much needed feature to the user.

There is some other problem in the pipeline that causes the resulting image to be distorted when presented, but that's unlikely a problem introduced by this patch.

aeikum commented 2 years ago

Hi @yshui, thanks for the PR. I've opened an internal issue to review your work. It's likely that there will be some changes needed before merging. Would you like us to provide a review of your work here, for you to continue improving; or would you prefer that we just adopt your work and make changes as necessary on our end?

yshui commented 2 years ago

@aeikum yes, please let me know if I need to change anything.

BTW, to support http, libsoup is needed as a dependency. I hope there won't be a problem introducing a new dependency into Proton?

nsivov commented 2 years ago

Hi, @yshui. One thing that needs to be reworked if importing winegstreamer from mf.dll. No reason here to justify private exports in this case. It's fine to duplicate scheme handler code in winegstreamer. Registration of network schemes should also go to winegstreamer, not mf.dll.

yshui commented 2 years ago

@nsivov I see. So I will copy the scheme handler code from mf to winegstreamer, is that OK?

yshui commented 2 years ago

@nsivov done.

nsivov commented 2 years ago

Thank you, will take a look soon.

nsivov commented 2 years ago

Thank you for decoupling it from mf.dll. What's left now is winegstreamer changes, @zfigura could you please take a look?

yshui commented 2 years ago

@nsivov @zfigura Hi, any updates?

yshui commented 2 years ago

hi, I rebased this PR. do you know how long would it take to review this PR? thanks for taking your time to look at this.

nsivov commented 2 years ago

Hi, I have no further comments on my part. I'm not really involved in ongoing winegstreamer changes, that's why I asked another reviewer to comment.

aeikum commented 2 years ago

Hi @yshui, I've been reviewing your patches here. It looks to me like Proton's gstreamer installation doesn't support HTTP URIs. On my host system, it appears to be using the libsoup-based HTTP URI handler, but we don't build that in Proton. Were you using Proton's gstreamer installation when developing this? If so, how did you get HTTP URIs to decode?

yshui commented 2 years ago

@aeikum Hi, thanks for looking at this PR! You are correct that it needs libsoup, I have a Proton fork that adds that. It's not yet in a mergeable state (e.g. I enabled all codec in ffmpeg for my own convenience), I plan to clean it up and open a PR to Proton after this one got through.

Besides libsoup, it also need glib-networking for TLS/SSL support, which needs to be provided by steam-runtime.

aeikum commented 2 years ago

Thanks. I think there are some other alternatives:

1) We could ask the runtime authors to provide libsoup (and its deps) for us, and update the Proton SDK.

2) We could implement these URI schemes on top of Win32 HTTP APIs, which would avoid the need for libsoup, etc.

I think implementing them with Win32 is the best option, but I haven't looked into how much work that would be. If it's really a lot of work, maybe getting libsoup in to the runtime and SDK is the best choice?

yshui commented 2 years ago

@aeikum Yeah, (2) is the proper way and is how the wine upstream wants this done. From the look of it, there will be 2 parts:

I would say (a) alone would require at least the same amount of work I put into this PR, most likely more. And (b) would be a much much bigger undertaking, judging from gstreamer's rtsp implementation.

Besides, merging this PR won't prevent us from doing this the proper way in the future.

OTOH, adding dependencies to the runtime sounds much easier. Except maybe the getting things approved by people aspect? I don't know what's this process like at Valve.

aeikum commented 2 years ago

Besides, merging this PR won't prevent us from doing this the proper way in the future.

Well, there are downsides to merging code that will just be replaced later. The time you spent implementing it, and us reviewing it, and Valve adding libsoup to the SRT, could instead have been spent on a proper implementation. Also when we switch implementations at some later date, it introduces a possibility for regressions, which makes for a crummy user experience. Not to say we shouldn't merge this, just saying there are downsides.

I've asked Valve to add libsoup to the SRT. That usually takes a week or two, then we can build the soup plugin in gst-plugins-good and merge in this branch.

yshui commented 2 years ago

@aeikum Yes, you said it very well, and I understand there is cost associated with this. I was just trying to clarify that all things considered I think this is worth it.

I've asked Valve to add libsoup to the SRT. That usually takes a week or two, then we can build the soup plugin in gst-plugins-good and merge in this branch.

Thank you so much!

yshui commented 2 years ago

I've asked Valve to add libsoup to the SRT.

BTW, also glib-networking is needed too for TLS/SSL

yshui commented 2 years ago

Update: resolved conflicts.

GloriousEggroll commented 2 years ago

It seems this is not working in current bleeding edge as of today with the latest changes + sdk. There's a couple broken things so ill explain:

abd632fd6eea09ca4c70fd13d9706f5530e18678 is broken from rbernon's group of patches. If I roll back to prior to that group:

9e9d6abfbb60ebf4d2d8d3825e6ca28861548a2c this works. If I then grab this PR and apply it on top, it breaks again (ghostwire tokyo videos completely fail, as well as borderlands 3, and 'the good life' videos do not play)

So currently rbernon's patches need to be fixed first, then this patchset needs to be fixed on top of it.

rbernon commented 2 years ago

You need to use the client_beta branch of the Steam Linux Runtime - Soldier app, the feature needs libsoup which isn't otherwise available in the runtime. Other than that this MR is included in Proton Experimental bleeding-edge branch already and as far as I can see it works fine (tested on The Good Life intro).

yshui commented 2 years ago

@aeikum @nsivov Looks like the commits have made it into the experimental_7.0 branch, thanks!

What would be the next steps from here? Should this PR be closed?

rbernon commented 2 years ago

Closing this as it has been merged. Thanks!