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

Various GLES state reconstruction fixes #2260

Merged
merged 6 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/memory/arena/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//core/context/keys:go_default_library",
"//core/data/compare:go_default_library",
],
)

Expand Down
8 changes: 8 additions & 0 deletions core/memory/arena/arena.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"fmt"
"unsafe"

"github.com/google/gapid/core/data/compare"

"github.com/google/gapid/core/context/keys"
)

Expand Down Expand Up @@ -216,3 +218,9 @@ func (r *Reader) Read(dst unsafe.Pointer, size int) {
}
r.Offset += size
}

func init() {
// Don't compare arenas.
compare.Register(func(c compare.Comparator, a, b Arena) {
})
}
4 changes: 1 addition & 3 deletions gapii/cc/spy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,11 @@ gapil::Ref<DynamicContextState> GlesSpy::GetEGLDynamicContextState(
display, context);
}

bool resetViewportScissor = true;
bool preserveBuffersOnSwap = swapBehavior == EGL_BUFFER_PRESERVED;

auto out = gapil::Ref<DynamicContextState>::create(
arena(), width, height, backbufferColorFmt, backbufferDepthFmt,
backbufferStencilFmt, resetViewportScissor, preserveBuffersOnSwap, r, g,
b, a, d, s);
backbufferStencilFmt, preserveBuffersOnSwap, r, g, b, a, d, s);

// Store the DynamicContextState as an extra.
observer->encode(*out.get());
Expand Down
8 changes: 8 additions & 0 deletions gapil/runtime/cc/maker.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ struct Maker<const void*, true> {
static inline void inplace_new(void** ptr, core::Arena* a) { *ptr = nullptr; }
};

// Special case for bool, because std::is_constructible<bool, core::Arena*>
// is true, but without an arg, we want it to return false.
template <>
struct Maker<bool, true> {
static inline bool make(core::Arena* a) { return false; }
static inline void inplace_new(bool* ptr, core::Arena* a) { *ptr = false; }
};

// make returns a T constructed by the list of args.
// If T has a core::Arena* as the first constructor parameter then a is
// prepended to the list of arguments.
Expand Down
1 change: 1 addition & 0 deletions gapis/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ go_library(
"//core/app/status:go_default_library",
"//core/context/keys:go_default_library",
"//core/data/binary:go_default_library",
"//core/data/compare:go_default_library",
"//core/data/deep:go_default_library",
"//core/data/endian:go_default_library",
"//core/data/generic:go_default_library",
Expand Down
6 changes: 4 additions & 2 deletions gapis/api/gles/api/egl.api
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,10 @@ cmd EGLBoolean eglMakeCurrent(EGLDisplay display,
SetContext(ctx)
if !ctx.Other.Initialized {
ApplyStaticContextState(ctx, GetEGLStaticContextState(display, context))
ApplyDynamicContextState(ctx, GetEGLDynamicContextState(display, draw, context), true)
} else {
ApplyDynamicContextState(ctx, GetEGLDynamicContextState(display, draw, context), false)
}
ApplyDynamicContextState(ctx, GetEGLDynamicContextState(display, draw, context))
} else {
dynamicState := GetEGLDynamicContextState(display, draw, context)
staticState := GetEGLStaticContextState(display, context)
Expand All @@ -300,7 +302,7 @@ cmd EGLBoolean eglMakeCurrent(EGLDisplay display,
// ApplyDynamicContextState() so SetContext() must be set before these
// calls.
ApplyStaticContextState(ctx, staticState)
ApplyDynamicContextState(ctx, dynamicState)
ApplyDynamicContextState(ctx, dynamicState, false)
} else {
// TODO: onEGLError(EGL_BAD_CONTEXT)
_ = newMsg(SEVERITY_ERROR, new!ERR_CONTEXT_DOES_NOT_EXIST(id: as!u64(context)))
Expand Down
1 change: 0 additions & 1 deletion gapis/api/gles/api/extras.api
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ class DynamicContextState {
GLenum BackbufferColorFmt
GLenum BackbufferDepthFmt
GLenum BackbufferStencilFmt
bool ResetViewportScissor
bool PreserveBuffersOnSwap
// TODO: Currently unused
@unused GLuint RedSize
Expand Down
4 changes: 1 addition & 3 deletions gapis/api/gles/custom_replay.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ func (ω *EglMakeCurrent) Mutate(ctx context.Context, id api.CmdID, s *api.Globa
return err
}
}
resetViewportScissor := false
if cs := FindDynamicContextState(s.Arena, ω.Extras()); !cs.IsNil() {
cmd := cb.ReplayChangeBackbuffer(
ctxID,
Expand All @@ -298,9 +297,8 @@ func (ω *EglMakeCurrent) Mutate(ctx context.Context, id api.CmdID, s *api.Globa
if err := cmd.Mutate(ctx, id, s, b, nil); err != nil {
return err
}
resetViewportScissor = cs.ResetViewportScissor()
}
if err := cb.ReplayBindRenderer(ctxID, resetViewportScissor).Mutate(ctx, id, s, b, nil); err != nil {
if err := cb.ReplayBindRenderer(ctxID, false).Mutate(ctx, id, s, b, nil); err != nil {
return err
}
return nil
Expand Down
4 changes: 2 additions & 2 deletions gapis/api/gles/gles.api
Original file line number Diff line number Diff line change
Expand Up @@ -740,7 +740,7 @@ sub void ApplyStaticContextState(ref!Context ctx, ref!StaticContextState staticS
}
}

sub void ApplyDynamicContextState(ref!Context ctx, ref!DynamicContextState dynamicState) {
sub void ApplyDynamicContextState(ref!Context ctx, ref!DynamicContextState dynamicState, bool resetViewportScissor) {
if (dynamicState != null) {
backbuffer := ctx.Objects.Framebuffers[0]

Expand All @@ -762,7 +762,7 @@ sub void ApplyDynamicContextState(ref!Context ctx, ref!DynamicContextState dynam
SizedFormat: dynamicState.BackbufferStencilFmt,
)

if dynamicState.ResetViewportScissor {
if resetViewportScissor {
ctx.Pixel.Scissor.Box.Width = dynamicState.BackbufferWidth
ctx.Pixel.Scissor.Box.Height = dynamicState.BackbufferHeight
ctx.Rasterization.Viewport.Width = dynamicState.BackbufferWidth
Expand Down
3 changes: 1 addition & 2 deletions gapis/api/gles/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,7 @@ func NewDynamicContextStateForTest(a arena.Arena, width, height int, preserveBuf
GLenum_GL_RGB565, // BackbufferColorFmt
GLenum_GL_DEPTH_COMPONENT16, // BackbufferDepthFmt
GLenum_GL_STENCIL_INDEX8, // BackbufferStencilFmt
true, // ResetViewportScissor
preserveBuffersOnSwap, // PreserveBuffersOnSwap
preserveBuffersOnSwap, // PreserveBuffersOnSwap
0, // RedSize
0, // GreenSize
0, // BlueSize
Expand Down
74 changes: 40 additions & 34 deletions gapis/api/gles/state_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,20 @@ func (API) RebuildState(ctx context.Context, oldState *api.GlobalState) ([]api.C
// Verify that the recreated state matches the original desired state.
diffs := 0
compare.Compare(s, GetState(sb.newState), func(d compare.Path) {
last := d[len(d)-1]
if oldSlice, ok := last.Value.(memory.Slice); ok {
if newSlice, ok := last.Value.(memory.Slice); ok {
old := AsU8ˢ(sb.tmpArena, oldSlice, sb.oldState.MemoryLayout)
new := AsU8ˢ(sb.tmpArena, newSlice, sb.newState.MemoryLayout)
if old.ResourceID(ctx, sb.oldState) == new.ResourceID(ctx, sb.newState) {
return // The pool IDs are different, but the resource IDs match exactly.
}
oldData := old.MustRead(ctx, nil, sb.oldState, nil)
newData := new.MustRead(ctx, nil, sb.newState, nil)
if reflect.DeepEqual(oldData, newData) {
return // The pool IDs are different, but the actual data matches exactly.
if l := len(d); l > 1 {
last := d[l-2] // l-1: is the pool, l-2 is the memory.Slice.
if oldSlice, ok := last.Reference.(memory.Slice); ok {
if newSlice, ok := last.Value.(memory.Slice); ok {
old := AsU8ˢ(sb.tmpArena, oldSlice, sb.oldState.MemoryLayout)
new := AsU8ˢ(sb.tmpArena, newSlice, sb.newState.MemoryLayout)
if old.ResourceID(ctx, sb.oldState) == new.ResourceID(ctx, sb.newState) {
return // The pool IDs are different, but the resource IDs match exactly.
}
oldData := old.MustRead(ctx, nil, sb.oldState, nil)
newData := new.MustRead(ctx, nil, sb.newState, nil)
if reflect.DeepEqual(oldData, newData) {
return // The pool IDs are different, but the actual data matches exactly.
}
}
}
}
Expand Down Expand Up @@ -294,6 +296,17 @@ func (sb *stateBuilder) contextObject(ctx context.Context, handle EGLContext, c
}
})
}
var programs []ProgramId
if objs := c.Objects().Programs(); sb.once(objs) && objs.Len() > 0 {
status.Do(ctx, fmt.Sprintf("rebuilding %d programs", objs.Len()), func(ctx context.Context) {
for _, id := range objs.Keys() {
if o := c.Objects().Programs().Get(id); !o.IsNil() {
sb.programObject(ctx, o)
programs = append(programs, id)
}
}
})
}
if objs := c.Objects().Shaders(); sb.once(objs) && objs.Len() > 0 {
status.Do(ctx, fmt.Sprintf("rebuilding %d shaders", objs.Len()), func(ctx context.Context) {
for _, id := range objs.Keys() {
Expand All @@ -303,19 +316,10 @@ func (sb *stateBuilder) contextObject(ctx context.Context, handle EGLContext, c
}
})
}
if objs := c.Objects().Programs(); sb.once(objs) && objs.Len() > 0 {
status.Do(ctx, fmt.Sprintf("rebuilding %d programs", objs.Len()), func(ctx context.Context) {
// Get the largest used shader ID.
maxID := ShaderId(0)
for i := range c.Objects().Shaders().All() {
if i > maxID {
maxID = i
}
}
for _, id := range objs.Keys() {
if o := c.Objects().Programs().Get(id); !o.IsNil() {
sb.programObject(ctx, o, uint32(maxID)+1)
}
if len(programs) > 0 {
status.Do(ctx, fmt.Sprintf("attaching shaders to %d programs", len(programs)), func(ctx context.Context) {
for _, id := range programs {
sb.attachShaders(ctx, c.Objects().Programs().Get(id))
}
})
}
Expand Down Expand Up @@ -674,7 +678,7 @@ func (sb *stateBuilder) shaderObject(ctx context.Context, s Shaderʳ) {
write(ctx, cb.GlShaderSource(id, 1, sb.readsData(ctx, sb.readsData(ctx, s.Source())), memory.Nullptr))
}

func (sb *stateBuilder) programObject(ctx context.Context, p Programʳ, firstShaderID uint32) {
func (sb *stateBuilder) programObject(ctx context.Context, p Programʳ) {
write, cb, id := sb.write, sb.cb, p.GetID()

write(ctx, cb.GlCreateProgram(id))
Expand Down Expand Up @@ -710,13 +714,11 @@ func (sb *stateBuilder) programObject(ctx context.Context, p Programʳ, firstSha
sb.E(ctx, "Precompiled programs not suppored yet") // TODO
continue
}
shaderID := ShaderId(firstShaderID)
firstShaderID++
write(ctx, cb.GlCreateShader(t, shaderID))
write(ctx, cb.GlShaderSource(shaderID, 1, sb.readsData(ctx, sb.readsData(ctx, s.Source())), memory.Nullptr))
write(ctx, api.WithExtras(cb.GlCompileShader(shaderID), s.Get().Clone(cb.Arena, sb.cloneCtx)))
write(ctx, cb.GlAttachShader(id, shaderID))
attachedShaders = append(attachedShaders, shaderID)
write(ctx, cb.GlCreateShader(t, s.ID()))
write(ctx, cb.GlShaderSource(s.ID(), 1, sb.readsData(ctx, sb.readsData(ctx, s.Source())), memory.Nullptr))
write(ctx, api.WithExtras(cb.GlCompileShader(s.ID()), s.Get().Clone(cb.Arena, sb.cloneCtx)))
write(ctx, cb.GlAttachShader(id, s.ID()))
attachedShaders = append(attachedShaders, s.ID())
}

write(ctx, api.WithExtras(cb.GlLinkProgram(id), p.LinkExtra().Get().Clone(cb.Arena, sb.cloneCtx)))
Expand All @@ -728,7 +730,7 @@ func (sb *stateBuilder) programObject(ctx context.Context, p Programʳ, firstSha
}
write(ctx, cb.GlUseProgram(0))

// Detach and delete the linked shaders (we'll attach the shaders from the state below).
// Detach and delete the linked shaders (we'll attach the shaders from the state later).
for _, shaderID := range attachedShaders {
write(ctx, cb.GlDetachShader(id, shaderID))
write(ctx, cb.GlDeleteShader(shaderID))
Expand All @@ -737,6 +739,10 @@ func (sb *stateBuilder) programObject(ctx context.Context, p Programʳ, firstSha
if !p.ValidateExtra().IsNil() {
write(ctx, api.WithExtras(cb.GlValidateProgram(id), p.ValidateExtra().Get().Clone(cb.Arena, sb.cloneCtx)))
}
}

func (sb *stateBuilder) attachShaders(ctx context.Context, p Programʳ) {
write, cb, id := sb.write, sb.cb, p.GetID()

for _, s := range p.Shaders().All() {
if s := s.GetID(); s != 0 {
Expand Down
8 changes: 8 additions & 0 deletions gapis/api/reference.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ package api

import (
"sync/atomic"

"github.com/google/gapid/core/data/compare"
)

// RefID is a type used to identify instances of the reference types used in the API models.
Expand Down Expand Up @@ -45,3 +47,9 @@ type NilReference struct{}
func (NilReference) RefID() RefID {
return NilRefID
}

func init() {
// Don't compare RefIDs.
compare.Register(func(c compare.Comparator, a, b RefID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change now, but we should probably avoid creating these global comparison functions - I've found they hid differences that you might want to find under different circumstances. The compare package needs a little bit of work to expose this, but we should probably be passing the custom rules down with each compare.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

})
}