Fix glGetQueryObjectiv call on devices which expect EXT variants.

Fix glGetQueryObjectiv call on devices which expect EXT variants.

R=sievers@chromium.org, vmiura@chromium.org
BUG=None
TEST=trybots

Committed: https://crrev.com/0fe97cb646bb8ba708fc5a2e23f7442ec36b4d4a
Cr-Commit-Position: refs/heads/master@{#312884}

[David Yen, 2015-01-21 23:03:13]

On 2015/01/21 23:01:29, David Yen wrote:

I noticed glGetQueryObjectiv is now defined as:

{ 'return_type': 'void',
  'versions': [{ 'name': 'glGetQueryObjectiv',
                 'gl_versions': ['gl3', 'gl4', 'es3'] }],
  'arguments': 'GLuint id, GLenum pname, GLint* params', },
{ 'return_type': 'void',
  'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
  'arguments': 'GLuint id, GLenum pname, GLint* params', },

In the GPUTracer it expected glGetQueryObjectiv() to collapse to
glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right thing
to use now?

[no sievers, 2015-01-21 23:09:40]

On 2015/01/21 23:03:13, David Yen wrote:
> On 2015/01/21 23:01:29, David Yen wrote:
> 
> I noticed glGetQueryObjectiv is now defined as:
> 
> { 'return_type': 'void',
>   'versions': [{ 'name': 'glGetQueryObjectiv',
>                  'gl_versions': ['gl3', 'gl4', 'es3'] }],
>   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> { 'return_type': 'void',
>   'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
>   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> 
> In the GPUTracer it expected glGetQueryObjectiv() to collapse to
> glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right thing
> to use now?

Yea, it's a bit of a mess. For example, the decoder always calls ARB variants
(see query_manager.cc).
Maybe we could just put the core, ARB, EXT query-related ones in the same
function pointer.
I'm actually in the process of rewriting the binding code and making it more
conditional (with preferred order of core over ARB over EXT). 

Also, on a related note:

GL_EXT_disjoint_timer_query uses GetInteger64v().
GetInteger64v() is only available on GLES3.
The extension is written against GLES2, so GetInteger64v() might or might not be
available.

[no sievers, 2015-01-21 23:12:12]

On 2015/01/21 23:09:40, sievers wrote:
> On 2015/01/21 23:03:13, David Yen wrote:
> > On 2015/01/21 23:01:29, David Yen wrote:
> > 
> > I noticed glGetQueryObjectiv is now defined as:
> > 
> > { 'return_type': 'void',
> >   'versions': [{ 'name': 'glGetQueryObjectiv',
> >                  'gl_versions': ['gl3', 'gl4', 'es3'] }],
> >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > { 'return_type': 'void',
> >   'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
> >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > 
> > In the GPUTracer it expected glGetQueryObjectiv() to collapse to
> > glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right
thing
> > to use now?
> 
> Yea, it's a bit of a mess. For example, the decoder always calls ARB variants
> (see query_manager.cc).
> Maybe we could just put the core, ARB, EXT query-related ones in the same
> function pointer.

I should say: and hope that it works!
Speaking of which: Did you notice any problems with incompatibilities of using
one entry point vs. another - given that the driver supports query-related
functionality (through GL core version or extensions) that exposes multiple
entry points (of for example core vs. ARB vs. EXT flavor)?
There could also be problem in how we resolve them: Right now the binding code
is very brute force (which I'm in the process of fixing). So you could
theoretically end up with a stub entry point that GetProcAddr() happened to
returned but is really invalid (like on Android it would be the case).

> I'm actually in the process of rewriting the binding code and making it more
> conditional (with preferred order of core over ARB over EXT). 
> 
> Also, on a related note:
> 
> GL_EXT_disjoint_timer_query uses GetInteger64v().
> GetInteger64v() is only available on GLES3.
> The extension is written against GLES2, so GetInteger64v() might or might not
be
> available.

[no sievers, 2015-01-21 23:24:45]

https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:183: glGetQueryObjectivARB(queries_[1],
GL_QUERY_RESULT_AVAILABLE, &done);
So ignoring everything we do in other places (like the decoder), I think the
current code is correct: For GL_ARB_timer_query at least which is written
against desktop GL 3.2, which will have this entry point.
For GLES, it should however be GetQueryObjectivEXT().

[David Yen, 2015-01-21 23:25:30]

On 2015/01/21 23:12:12, sievers wrote:
> On 2015/01/21 23:09:40, sievers wrote:
> > On 2015/01/21 23:03:13, David Yen wrote:
> > > On 2015/01/21 23:01:29, David Yen wrote:
> > > 
> > > I noticed glGetQueryObjectiv is now defined as:
> > > 
> > > { 'return_type': 'void',
> > >   'versions': [{ 'name': 'glGetQueryObjectiv',
> > >                  'gl_versions': ['gl3', 'gl4', 'es3'] }],
> > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > { 'return_type': 'void',
> > >   'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
> > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > 
> > > In the GPUTracer it expected glGetQueryObjectiv() to collapse to
> > > glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right
> thing
> > > to use now?
> > 
> > Yea, it's a bit of a mess. For example, the decoder always calls ARB
variants
> > (see query_manager.cc).
> > Maybe we could just put the core, ARB, EXT query-related ones in the same
> > function pointer.
> 
> I should say: and hope that it works!
> Speaking of which: Did you notice any problems with incompatibilities of using
> one entry point vs. another - given that the driver supports query-related
> functionality (through GL core version or extensions) that exposes multiple
> entry points (of for example core vs. ARB vs. EXT flavor)?
> There could also be problem in how we resolve them: Right now the binding code
> is very brute force (which I'm in the process of fixing). So you could
> theoretically end up with a stub entry point that GetProcAddr() happened to
> returned but is really invalid (like on Android it would be the case).
> 


Yes, currently that it of code crashes on android as soon as it calls
glGetQueryObjectiv(). Took me a bit of time but tracked it down to that change
:)


> > I'm actually in the process of rewriting the binding code and making it more
> > conditional (with preferred order of core over ARB over EXT). 
> > 
> > Also, on a related note:
> > 
> > GL_EXT_disjoint_timer_query uses GetInteger64v().
> > GetInteger64v() is only available on GLES3.
> > The extension is written against GLES2, so GetInteger64v() might or might
not
> be
> > available.

I believe in the specs for the ARB_Timer extension it adds GetInteger64v with
GL_TIMESTAMP:

https://www.opengl.org/registry/specs/ARB/timer_query.txt

In fact, we should be able to change line 461-473 to just use glGetInteger64v as
well.

[no sievers, 2015-01-21 23:29:12]

On 2015/01/21 23:25:30, David Yen wrote:
> On 2015/01/21 23:12:12, sievers wrote:
> > On 2015/01/21 23:09:40, sievers wrote:
> > > On 2015/01/21 23:03:13, David Yen wrote:
> > > > On 2015/01/21 23:01:29, David Yen wrote:
> > > > 
> > > > I noticed glGetQueryObjectiv is now defined as:
> > > > 
> > > > { 'return_type': 'void',
> > > >   'versions': [{ 'name': 'glGetQueryObjectiv',
> > > >                  'gl_versions': ['gl3', 'gl4', 'es3'] }],
> > > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > > { 'return_type': 'void',
> > > >   'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
> > > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > > 
> > > > In the GPUTracer it expected glGetQueryObjectiv() to collapse to
> > > > glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right
> > thing
> > > > to use now?
> > > 
> > > Yea, it's a bit of a mess. For example, the decoder always calls ARB
> variants
> > > (see query_manager.cc).
> > > Maybe we could just put the core, ARB, EXT query-related ones in the same
> > > function pointer.
> > 
> > I should say: and hope that it works!
> > Speaking of which: Did you notice any problems with incompatibilities of
using
> > one entry point vs. another - given that the driver supports query-related
> > functionality (through GL core version or extensions) that exposes multiple
> > entry points (of for example core vs. ARB vs. EXT flavor)?
> > There could also be problem in how we resolve them: Right now the binding
code
> > is very brute force (which I'm in the process of fixing). So you could
> > theoretically end up with a stub entry point that GetProcAddr() happened to
> > returned but is really invalid (like on Android it would be the case).
> > 
> 
> 
> Yes, currently that it of code crashes on android as soon as it calls
> glGetQueryObjectiv(). Took me a bit of time but tracked it down to that change
> :)
> 
> 
> > > I'm actually in the process of rewriting the binding code and making it
more
> > > conditional (with preferred order of core over ARB over EXT). 
> > > 
> > > Also, on a related note:
> > > 
> > > GL_EXT_disjoint_timer_query uses GetInteger64v().
> > > GetInteger64v() is only available on GLES3.
> > > The extension is written against GLES2, so GetInteger64v() might or might
> not
> > be
> > > available.
> 
> I believe in the specs for the ARB_Timer extension it adds GetInteger64v with
> GL_TIMESTAMP:
> 
> https://www.opengl.org/registry/specs/ARB/timer_query.txt
> 

Now you are mixing up the two. You are calling GetInteger64v on GLES
(kTracerTypeDisjointTimer).
ARB_timer_query is a desktop extension, and you wouldn't know if you have this
extension (timer_query) anyway when you created the kTracerTypeDisjointTimer
tracer.

> In fact, we should be able to change line 461-473 to just use glGetInteger64v
as
> well.

[no sievers, 2015-01-21 23:31:16]

https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:147: glGetInteger64v(GL_TIMESTAMP,
&gl_now);
Am talking about this call site.

[David Yen, 2015-01-21 23:35:13]

On 2015/01/21 23:31:16, sievers wrote:
>
https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/gpu_tracer.cc (right):
> 
>
https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/gpu_tracer.cc:147: glGetInteger64v(GL_TIMESTAMP,
> &gl_now);
> Am talking about this call site.

Sorry, I used the ARB_Timer as a base because the disjoint timer is an extension
of the ARB_Timer (it is the ARB_Timer with an extra disjoint call). You are
right that technically they are defined separately.

The GetInteger64v addition with GL_TIMESTAMP_EXT is also mentioned in the
disjoint timer documentation (GL_TIMESTAMP and GL_TIMESTAMP_EXT have the same
value):

https://www.khronos.org/registry/gles/extensions/EXT/EXT_disjoint_timer_query...

This should work anywhere that has that extension then right?

[no sievers, 2015-01-21 23:36:50]

On 2015/01/21 23:25:30, David Yen wrote:
> On 2015/01/21 23:12:12, sievers wrote:
> > On 2015/01/21 23:09:40, sievers wrote:
> > > On 2015/01/21 23:03:13, David Yen wrote:
> > > > On 2015/01/21 23:01:29, David Yen wrote:
> > > > 
> > > > I noticed glGetQueryObjectiv is now defined as:
> > > > 
> > > > { 'return_type': 'void',
> > > >   'versions': [{ 'name': 'glGetQueryObjectiv',
> > > >                  'gl_versions': ['gl3', 'gl4', 'es3'] }],
> > > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > > { 'return_type': 'void',
> > > >   'names': ['glGetQueryObjectivARB', 'glGetQueryObjectivEXT'],
> > > >   'arguments': 'GLuint id, GLenum pname, GLint* params', },
> > > > 
> > > > In the GPUTracer it expected glGetQueryObjectiv() to collapse to
> > > > glGetQueryObjectEXT(). I assume using glGetQueryObjectARB() is the right
> > thing
> > > > to use now?
> > > 
> > > Yea, it's a bit of a mess. For example, the decoder always calls ARB
> variants
> > > (see query_manager.cc).
> > > Maybe we could just put the core, ARB, EXT query-related ones in the same
> > > function pointer.
> > 
> > I should say: and hope that it works!
> > Speaking of which: Did you notice any problems with incompatibilities of
using
> > one entry point vs. another - given that the driver supports query-related
> > functionality (through GL core version or extensions) that exposes multiple
> > entry points (of for example core vs. ARB vs. EXT flavor)?
> > There could also be problem in how we resolve them: Right now the binding
code
> > is very brute force (which I'm in the process of fixing). So you could
> > theoretically end up with a stub entry point that GetProcAddr() happened to
> > returned but is really invalid (like on Android it would be the case).
> > 
> 
> 
> Yes, currently that it of code crashes on android as soon as it calls
> glGetQueryObjectiv(). Took me a bit of time but tracked it down to that change
> :)
> 
> 
> > > I'm actually in the process of rewriting the binding code and making it
more
> > > conditional (with preferred order of core over ARB over EXT). 
> > > 
> > > Also, on a related note:
> > > 
> > > GL_EXT_disjoint_timer_query uses GetInteger64v().
> > > GetInteger64v() is only available on GLES3.
> > > The extension is written against GLES2, so GetInteger64v() might or might
> not
> > be
> > > available.
> 
> I believe in the specs for the ARB_Timer extension it adds GetInteger64v with
> GL_TIMESTAMP:

This is also not true on its own. You still need Open GL 3.2 or better.


> 
> https://www.opengl.org/registry/specs/ARB/timer_query.txt
> 
> In fact, we should be able to change line 461-473 to just use glGetInteger64v
as
> well.

[no sievers, 2015-01-21 23:42:45]

On 2015/01/21 23:35:13, David Yen wrote:
> On 2015/01/21 23:31:16, sievers wrote:
> >
>
https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
> > File gpu/command_buffer/service/gpu_tracer.cc (right):
> > 
> >
>
https://codereview.chromium.org/866593002/diff/20001/gpu/command_buffer/servi...
> > gpu/command_buffer/service/gpu_tracer.cc:147: glGetInteger64v(GL_TIMESTAMP,
> > &gl_now);
> > Am talking about this call site.
> 
> Sorry, I used the ARB_Timer as a base because the disjoint timer is an
extension
> of the ARB_Timer (it is the ARB_Timer with an extra disjoint call). You are
> right that technically they are defined separately.
> 

Be careful, one is written against OpenGL, one is written against GLES.
They are different things. One is not an extension of the other.

> The GetInteger64v addition with GL_TIMESTAMP_EXT is also mentioned in the
> disjoint timer documentation (GL_TIMESTAMP and GL_TIMESTAMP_EXT have the same
> value):
> 
>
https://www.khronos.org/registry/gles/extensions/EXT/EXT_disjoint_timer_query...
> 
> This should work anywhere that has that extension then right?

It doesn't specify GetInteger64v under 'New procedures and functions' in the
extension spec.
And it's written against GLES 2.0. So I *think* you would have to check the GLES
version also. It's a bit confusing in this case.

[no sievers, 2015-01-21 23:50:52]

I think the simple fix would be to support two cases:
- desktop 3.2 and GL_ARB_timer_query (the latter probably implies the former)
- GLES 3.0 or higher and disjoint timer

If you don't support creating a tracer on GLES2, then that fixes the
GetInteger64v problem.

For glGetQueryObjectiv you can change it to call glGetQueryObjectivEXT for the
disjoint instantiation. For desktop, keep calling glGetQueryObjectiv().

[David Yen, 2015-01-22 19:19:53]

On 2015/01/21 23:50:52, sievers wrote:
> I think the simple fix would be to support two cases:
> - desktop 3.2 and GL_ARB_timer_query (the latter probably implies the former)
> - GLES 3.0 or higher and disjoint timer
> 
> If you don't support creating a tracer on GLES2, then that fixes the
> GetInteger64v problem.
> 
> For glGetQueryObjectiv you can change it to call glGetQueryObjectivEXT for the
> disjoint instantiation. For desktop, keep calling glGetQueryObjectiv().

I've added the check for ES3 when using the disjoint timer. As we discussed
glGenQueries and glDelQueries, and glGetQueryOBjectiv all use the ARB variants,
but this should be changed to the core version once the binding refactor is
completed.

Since GL_ARB_timer_query is based on Open GL 3.2, I also changed the complicated
timing query to simply use glGetInteger64v().

PTAL.

[no sievers, 2015-01-22 19:51:20]

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:456: return;
nit: 
else if (!gpu_timing_synced_ && tracer_type == kTracerTypeARBTimer) { 
  ...
}

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.h:101: virtual GpuTracerType
DetermineTracerType(gles2::GLES2Decoder* decoder);
Why pass in the decoder (instead of simply accesssing |decoder_|)?

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:334: .Times(Exactly(0));
You can create a StrictMock instance, then you don't have to do the Times(0)
thing here and above. See gles2_cmd_decoder_unittest.cc

[David Yen, 2015-01-22 21:43:02]

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.cc:456: return;
On 2015/01/22 19:51:20, sievers wrote:
> nit: 
> else if (!gpu_timing_synced_ && tracer_type == kTracerTypeARBTimer) { 
>   ...
> }

Done.

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer.h (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer.h:101: virtual GpuTracerType
DetermineTracerType(gles2::GLES2Decoder* decoder);
On 2015/01/22 19:51:20, sievers wrote:
> Why pass in the decoder (instead of simply accesssing |decoder_|)?

Ah, I had a brain fart. Done.

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:334: .Times(Exactly(0));
On 2015/01/22 19:51:20, sievers wrote:
> You can create a StrictMock instance, then you don't have to do the Times(0)
> thing here and above. See gles2_cmd_decoder_unittest.cc

It actually already using a StrictMock (see gpu_service_test.cc:30). This is
actually testing something a bit more subtle. It's not that GetInteger64v cannot
be called, it's that it must be called at the right place.

So for the disjoint timer after ExpectTracerOffsetQueryMocks() setups up the
tracer mock, GetInteger64v is not allowed to be called (disjoint timers needs to
sychronize every trace). However, after ExpectTraceQueryMocks() setups the trace
mocks it is then allowed again (by calling EXPECT_CALL on GetInteger64v).

[no sievers, 2015-01-23 01:21:29]

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:334: .Times(Exactly(0));
On 2015/01/22 21:43:02, David Yen wrote:
> On 2015/01/22 19:51:20, sievers wrote:
> > You can create a StrictMock instance, then you don't have to do the Times(0)
> > thing here and above. See gles2_cmd_decoder_unittest.cc
> 
> It actually already using a StrictMock (see gpu_service_test.cc:30). This is
> actually testing something a bit more subtle. It's not that GetInteger64v
cannot
> be called, it's that it must be called at the right place.
> 
> So for the disjoint timer after ExpectTracerOffsetQueryMocks() setups up the
> tracer mock, GetInteger64v is not allowed to be called (disjoint timers needs
to
> sychronize every trace). However, after ExpectTraceQueryMocks() setups the
trace
> mocks it is then allowed again (by calling EXPECT_CALL on GetInteger64v).

You can use InSequence to guarantee order. I actually think it should always be
used unless there is a good reason not to :)

[no sievers, 2015-01-23 01:30:40]

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gpu_tracer_unittest.cc (right):

https://codereview.chromium.org/866593002/diff/40001/gpu/command_buffer/servi...
gpu/command_buffer/service/gpu_tracer_unittest.cc:334: .Times(Exactly(0));
On 2015/01/23 01:21:29, sievers wrote:
> On 2015/01/22 21:43:02, David Yen wrote:
> > On 2015/01/22 19:51:20, sievers wrote:
> > > You can create a StrictMock instance, then you don't have to do the
Times(0)
> > > thing here and above. See gles2_cmd_decoder_unittest.cc
> > 
> > It actually already using a StrictMock (see gpu_service_test.cc:30). This is
> > actually testing something a bit more subtle. It's not that GetInteger64v
> cannot
> > be called, it's that it must be called at the right place.
> > 
> > So for the disjoint timer after ExpectTracerOffsetQueryMocks() setups up the
> > tracer mock, GetInteger64v is not allowed to be called (disjoint timers
needs
> to
> > sychronize every trace). However, after ExpectTraceQueryMocks() setups the
> trace
> > mocks it is then allowed again (by calling EXPECT_CALL on GetInteger64v).
> 
> You can use InSequence to guarantee order. I actually think it should always
be
> used unless there is a good reason not to :)

lgtm btw, that's just a nit in case you want to try make it more
straightforward.

[David Yen, 2015-01-23 17:29:31]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-01-23 17:30:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/866593002/60001

[commit-bot: I haz the power, 2015-01-23 18:47:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-23 18:48:29]

Patchset 4 (id:??) landed as
https://crrev.com/0fe97cb646bb8ba708fc5a2e23f7442ec36b4d4a
Cr-Commit-Position: refs/heads/master@{#312884}