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

Fixed setprop after using VkGraphicsSpy #1051

Closed
wants to merge 1 commit into from

Conversation

sjfricke
Copy link
Contributor

@sjfricke sjfricke commented Sep 1, 2017

#1049

I found this actually lets me trace the Android Vulkan Tutorials part 5 and also doesn't break all other Vulkan apps.

I feel this should be verified across multiple devices/OS before merging

@sjfricke
Copy link
Contributor Author

sjfricke commented Sep 1, 2017

Ubuntu with Galaxy phones verified

@AWoloszyn
Copy link
Contributor

So this actually fails for me.
Ubuntu + Pixel C.

The call to adb shell setprop

adb shell setprop debug.vulkan.layers ""
usage: setprop NAME VALUE

Sets an Android system property.

setprop: Need 2 arguments

I am guessing this is a difference between Samsung and Pixel devices.
If we appended the other type of clear, that should handle both cases.

		// setprop has slightly different semantics on different devices.
		// First try to clear with escaped quotes, which will leave the property
		// set as "" on some devices.
		// Then clear with non-escaped quotes, this clear it on the devices that
		// would have set it as "", but simply fail on others.
		d.Command("shell", "setprop", "debug.vulkan.layers", "\"\"").Run(ctx)
		d.Command("shell", "setprop", "debug.vulkan.layers", "").Run(ctx)

@sjfricke
Copy link
Contributor Author

sjfricke commented Sep 1, 2017

I can't tell if it's the Go code or the phone, when it clears and you go adb shell getprop | grep vulkan do you get [ ] or [""]

Also i'd wait til you can confirm your S6 is the same as I feel having it prompt the setprop: Need 2 arguments error should be avoided unless this is the case that Pixel and Samsung devices have a different set method because going adb shell setprop debug.vulkan.layers "" works on my Galaxies, just seems the \"\" aren't escaping correctly

@AWoloszyn
Copy link
Contributor

So with the existing implementation I get
[debug.vulkan.layers]: [] on both my PixelC and my Pixel phone.

@sjfricke
Copy link
Contributor Author

sjfricke commented Sep 2, 2017

So this is not a phone thing, I got gapid up and running from tip or tree on my Windows 10 laptop and it ran the trace and when it was done it set [debug.vulkan.layers]: [] properly.

Made gist to test with

  • With go version go1.8.3 windows/amd64 on Galaxy S8 the "\"\"" method works
  • With go version go1.8 linux/amd64 on Galaxy S8 the "" method works

Both running Android Platform-Tools v26.0.0

What I don't get is for my Ubuntu version it literally fmt.prints adb shell setprop debug.vulkan.layers "" but sets it as [""] and then if I copy and paste the stdout back into the command line and run it turns it back to [ ] Like I honestly don't know what character encoding issue this is, but this is the heart of the issue right here

@AWoloszyn
Copy link
Contributor

I just got an S6 to test with.

go version go1.8 linux/amd64 -> "\"\"" works for me
"" fails with FAILED: adb shell setprop debug.vulkan.layers : exit status 1

adb version                                                                                                                                                               
Android Debug Bridge version 1.0.36
...

adb shell getprop | grep ro.build.description                                                                                                                             
[ro.build.description]: [zerofltebmc-user 7.0 NRD90M G920W8VLS5DQH1 release-keys]

@sjfricke sjfricke closed this Sep 5, 2017
purvisa-at-google-com pushed a commit that referenced this pull request Sep 29, 2022
It used to be an image but now it's an actual button
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.

2 participants