Record the content of external images during trace #1103
Conversation
dsrbecky
force-pushed
the
external-image
branch
from
40f0c7e to
d58c3d4
Compare
gapis/api/gles/compat.go
Outdated
|
|
||
| // Update the content if we made a snapshot. | ||
| if e := FindEGLImageData(cmd.Extras()); e != nil { | ||
| _, err := database.Resolve(ctx, e.ID) |
There was a problem hiding this comment.
This check currently seems to fail, any idea why the resource can not be found?
I failed to debug it so far as the IDs generally print as random ascii art. Can we just have incrementing integers for the resource IDs please?
There was a problem hiding this comment.
This check currently seems to fail, any idea why the resource can not be found?
Yes.
The resource IDs are generated in GAPII using cityhash. As a memory range is observed, the hash is calculated, and used to determine if the resource has already been encoded. If not, the resource data and hash is encoded in the Capture::Resource message. An observation always refers to the resource data by this hash.
GAPIS uses SHA1 hashes for all database entries. When you store some data, the message is encoded to binary, SHA1 hashed, and checked to see if it is already in the database. We use this to ensure we only have one copy of any data and also for ensuring that Resolvable types always do the work once.
SHA1 is moderately expensive to calculate, hence we use the much faster cityhash in the interceptor. SHA1 is a better hashing function, so we have used it for the server.
So, to avoid having to deal with a cityhash->SHA1 map after load, we replace the cityhash resource hashes with SHA hashes while we're deserializing.
Memory observations are explicitly handled, but we now have your new type that is trying to load the resources from the database using the cityhash IDs.
My recommendation for now would be to generalize the hash remapping logic so you can remap the IDs in the EGLImageData.
Consider creating this interface in the api package:
// ObservationReference is the interface implemented by types that hold references
// to Observation IDs. As these IDs are remapped on load, references must update
// any IDs using the map passed to RemapObservationIDs.
type ObservationReference interface {
RemapObservationIDs(map[id.ID]id.ID)
}Then you can check for this interface here and call it.
And move this functionality into api.CmdObservation.RemapObservationIDs(). You'll then be able to do the same for your own new type.
I failed to debug it so far as the IDs generally print as random ascii art.
id.ID should print just fine. Are you referring to printing from the GAPII side?
Can we just have incrementing integers for the resource IDs please?
Which IDs are you proposing to change? The IDs for Observations in the .gfxtrace file? Yes, that could work, but would break file formats again.
If you're referring to all IDs, that breaks a whole bunch of fundamental assumptions, and would be a massive change.
There was a problem hiding this comment.
Created #1105 to give you this interface.
gapis/api/gles/extras.go
Outdated
| @@ -30,6 +31,16 @@ type ErrorState struct { | |||
| InterceptorsGlError GLenum | |||
| } | |||
|
|
|||
| // EGLImageData is an extra used to store snapshot of external image source. | |||
| type EGLImageData struct { | |||
There was a problem hiding this comment.
It's fine to have this type here, but you could just use the proto directly.
Used to let deserialized objects remap their Resource identifiers to those returned by database.Store. See google#1103 for more info.
Used to let deserialized objects remap their Resource identifiers to those returned by database.Store. See #1103 for more info.
ben-clayton
force-pushed
the
external-image
branch
from
d58c3d4 to
b5082f9
Compare
dsrbecky
force-pushed
the
external-image
branch
2 times, most recently
from
053c624 to
f34fd78
Compare
gapis/api/gles/gles_pb/extras.proto
Outdated
| @@ -22,3 +22,13 @@ message ErrorState { | |||
| uint32 TraceDriversGlError = 1; | |||
| uint32 InterceptorsGlError = 2; | |||
| } | |||
|
|
|||
| message EGLImageData { | |||
| string ID = 1; // Resource ID of the image data | |||
There was a problem hiding this comment.
Nit: could just use bytes to avoid a ID parse
dsrbecky
force-pushed
the
external-image
branch
from
f34fd78 to
e9aafc8
Compare
Adds an error code to the validation result to help distinguish between different types of validation errors.
No description provided.