Add Stadia Support. #2732
Add Stadia Support. #2732
Conversation
AWoloszyn
requested review from
pmuetschard,
ben-clayton and
pau-baiget
and removed request for
ben-clayton
|
@ben-clayton I have responded to all comments. Thanks! |
AWoloszyn
force-pushed
the
stadia
branch
2 times, most recently
from
6bf30fc to
55ee76a
Compare
| return ret, nil | ||
| } | ||
|
|
||
| // ParseListOutput parses the output from ggp xxx list command. |
There was a problem hiding this comment.
ggp xxx list -s gives the output as JSON, would it be better to use this and a proper JSON parser?
If we stick with plain text, maybe add a small example of the kind of text that is parsed? e.g.
blablabla
header1 header2 header3
======= ======= =======
value1 value2 value3
There was a problem hiding this comment.
I added the example, there is a good reason (for now) that we support this option. We can remove in the future. I can elaborate offline if you would like.
| // corresponding return value. One special case only used internally is: | ||
| // "package=/", which returns proj="/", pkg="", app="". | ||
| func parsePackageURI(ctx context.Context, uri string) (proj, pkg, app string, err error) { | ||
| re := regexp.MustCompile(`package=(\/$|[a-zA-Z0-9\_\-\s]+\/)?([a-z0-9\_\-\s]+)?(\:[a-zA-Z0-9\s\_\-]+$)?`) |
There was a problem hiding this comment.
Nit: not sure regexp is the most maintainable approach here, but I guess it does the job.
Another approach would be to tokenize the URI using ['/', '=', ':'] as delimiters and analyse the resulting split.
There was a problem hiding this comment.
The allowable tokens are fairly well-defined, so I think the regex is okay at the moment. If we run into issues, we can split it out more carefully.
|
Updated from comments. |
|
@pmuetschard @hevrard Would either of you be able to review/approve this? I would like to avoid waiting too much longer, and having to rebase. |
There was a problem hiding this comment.
There are still a bunch of unaddressed comments from @hevrard
core/os/device/ggp/device.go
Outdated
| var lastErrorPrinted time.Time | ||
|
|
||
| for { | ||
| if err := func() error { |
There was a problem hiding this comment.
This whole closure looks pointless.
Isn't it the same as:
configs, err := getConfigs(ctx)
if err != nil {
return err
}
if err := scanDevices(ctx, configs); err != nil {
if time.Since(lastErrorPrinted).Seconds() > printScanErrorsEveryNSeconds {
log.E(ctx, "Couldn't scan devices: %v", err)
lastErrorPrinted = time.Now()
}
} else {
lastErrorPrinted = time.Time{}
}| @@ -393,6 +397,10 @@ bool Vulkan::replayCreateVkDeviceImpl(Stack* stack, size_val physicalDevice, | |||
| xcbInfo.window = info->window; | |||
| } | |||
| break; | |||
| case gapir::SurfaceType::Ggp: | |||
There was a problem hiding this comment.
Indentation is off, which is quite confusing.
There was a problem hiding this comment.
Still off compared to L391.
|
Thanks, I will update. They got themselves buried I guess. |
| @@ -368,7 +369,10 @@ bool Vulkan::replayCreateVkDeviceImpl(Stack* stack, size_val physicalDevice, | |||
| case VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR: | |||
| target = gapir::SurfaceType::Xcb; | |||
| break; | |||
| case VK_STRUCTURE_TYPE_WIN32_SURFACE_CREATE_INFO_KHR: | |||
| case VK_STRUCTURE_TYPE_STREAM_DESCRIPTOR_SURFACE_CREATE_INFO_GGP: | |||
| @@ -379,22 +383,27 @@ bool Vulkan::replayCreateVkDeviceImpl(Stack* stack, size_val physicalDevice, | |||
| switch (target) { | |||
| #if TARGET_OS == GAPID_OS_ANDROID | |||
| case gapir::SurfaceType::Android: | |||
| createInfo = &androidInfo; | |||
| @@ -393,6 +397,10 @@ bool Vulkan::replayCreateVkDeviceImpl(Stack* stack, size_val physicalDevice, | |||
| xcbInfo.window = info->window; | |||
| } | |||
| break; | |||
| case gapir::SurfaceType::Ggp: | |||
There was a problem hiding this comment.
Still off compared to L391.
|
Fixed formatting, will merge once builds go green. |
No description provided.