1. support stop mode for omx

1. support stop mode for omx
2. make player_x11 respond to button event quickly

BUG=none
TEST=dev platform

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49749

[wjia(left Chromium), 2010-06-10 23:25:38]

Now OMX supports "stop" (both eglimage and system memory buffer cases). Somehow
ffmpeg case crashes. I guess something is wrong with ffmpeg decoder since
rendering part is shared with omx system memory buffer case.

[jiesun, 2010-06-11 16:36:49]

are we going to address the FillBufferDone and FreeOutputBuffer race in this
patch?
could we debug ffmpeg path a little bit?
is Stoped state different from uninit state? if not , or very trivial , we could
merge state.
thanks

http://codereview.chromium.org/2781010/diff/1/4
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/2781010/diff/1/4#newcode144
media/tools/player_x11/player_x11.cc:144: message_loop->QuitNow();
what's the concern here?

[wjia(left Chromium), 2010-06-11 16:59:46]

The FillBufferDone and FreeOutputBuffer race has been addressed by checking
CanAcceptOutput(). It's different from handling of input. This is just a
different handling. We could use similar handling to input.

I will address ffmpeg decoder after this patch.

Stopped state is different from UnInit state. In stopped state, we don't accept
any output buffer, while in UnInit state, we do.

http://codereview.chromium.org/2781010/diff/1/4
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/2781010/diff/1/4#newcode144
media/tools/player_x11/player_x11.cc:144: message_loop->QuitNow();
Quit() will wait for all pending task done, while QuitNow() doesn't. In this
case, video renderer base keeps adding task  to the message_loop and that will
prevent quitting. We have to use QuitNow() if we do want to quit immediately.

On 2010/06/11 16:36:49, jiesun wrote:
> what's the concern here?

[jiesun, 2010-06-11 18:37:43]

could we put this in comments rather than code-review?
"Stopped state is different from UnInit state. In stopped state, we don't accept
any output buffer, while in UnInit state, we do."

[jiesun, 2010-06-11 18:38:02]

http://codereview.chromium.org/2781010/diff/1/4
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/2781010/diff/1/4#newcode145
media/tools/player_x11/player_x11.cc:145: return;
I believe a lot of logic ( include clean up task ) is done in message loop, (
because of threading issue). it may not wise to QuitNow() to leave the mess
behind.
how do you think. could the task be stopped from render? or could you explain
more? are Render stopped asynchronously or it had to stop after decoder stopped?

[Alpha Left Google, 2010-06-11 18:57:07]

http://codereview.chromium.org/2781010/diff/1/2
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/2781010/diff/1/2#newcode172
media/filters/omx_video_decode_engine.cc:172: case kClientStopped:
Now kClientStopped means kStopped, shouldn't kClientStopping equals to kNormal?

http://codereview.chromium.org/2781010/diff/1/2#newcode528
media/filters/omx_video_decode_engine.cc:528: return;
Need to call the callback before returning. Otherwise the buffer ownership is
not returned to the renderer.

http://codereview.chromium.org/2781010/diff/1/4
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/2781010/diff/1/4#newcode144
media/tools/player_x11/player_x11.cc:144: message_loop->QuitNow();
On 2010/06/11 16:59:46, wjia wrote:
> Quit() will wait for all pending task done, while QuitNow() doesn't. In this
> case, video renderer base keeps adding task  to the message_loop and that will
> prevent quitting. We have to use QuitNow() if we do want to quit immediately.
> 
> On 2010/06/11 16:36:49, jiesun wrote:
> > what's the concern here?
> 
> 

Actually doing either Quit() or QuitNow() have problems. If the EGL context or
GL context is only accessible on the glx_thread (i.e. message_loop) by shutting
down this thread we can't do uninitialization properly.

We can call pipeline->Stop(NULL) first that shutdown all the filters,
glx_renderer will destroy textures and resources and complete the asynchronous
stop. This does not happen because we run the main loop the same message loop as
glx renderer, in order to do this correctly, we need to:

1. Create a new thread called glx_thread, pass the message loop of this thread
to the renderer.

2. Host all X calls on this glx_thread.

3. Check for X events on this glx_thread. If shutdown is needed, post a task to
main message loop to call pipeline->Stop(). And when Stop() returns we can do
XDestroyWindow() on this glx_thread, we simply run message->Quit().

4. Implement asynchronous stop in the renderers.

[wjia(left Chromium), 2010-06-14 21:09:46]

http://codereview.chromium.org/2781010/diff/1/2
File media/filters/omx_video_decode_engine.cc (right):

http://codereview.chromium.org/2781010/diff/1/2#newcode172
media/filters/omx_video_decode_engine.cc:172: case kClientStopped:
On 2010/06/11 18:57:08, Alpha wrote:
> Now kClientStopped means kStopped, shouldn't kClientStopping equals to
kNormal?

According to previous logic (before merge of omx_codec and engine),
kClientStopping means kStopped.

http://codereview.chromium.org/2781010/diff/1/2#newcode528
media/filters/omx_video_decode_engine.cc:528: return;
Done. The buffer owner has one ref count on it. This returning is not really
needed though.

On 2010/06/11 18:57:08, Alpha wrote:
> Need to call the callback before returning. Otherwise the buffer ownership is
> not returned to the renderer.

http://codereview.chromium.org/2781010/diff/1/4
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/2781010/diff/1/4#newcode144
media/tools/player_x11/player_x11.cc:144: message_loop->QuitNow();
Calling QuitNow() works fine as long as we wait for pipeline to be fully stopped
when calling pipeline->stop(...) as

base::WaitableEvent event(false, false);
pipeline->Stop(NewCallback(&event, &base::WaitableEvent::Signal));
event.Wait();

The thread won't stop till pipeline is fully stopped. After that, X Window will
be shutdown.

On 2010/06/11 18:57:08, Alpha wrote:
> On 2010/06/11 16:59:46, wjia wrote:
> > Quit() will wait for all pending task done, while QuitNow() doesn't. In this
> > case, video renderer base keeps adding task  to the message_loop and that
will
> > prevent quitting. We have to use QuitNow() if we do want to quit
immediately.
> > 
> > On 2010/06/11 16:36:49, jiesun wrote:
> > > what's the concern here?
> > 
> > 
> 
> Actually doing either Quit() or QuitNow() have problems. If the EGL context or
> GL context is only accessible on the glx_thread (i.e. message_loop) by
shutting
> down this thread we can't do uninitialization properly.
> 
> We can call pipeline->Stop(NULL) first that shutdown all the filters,
> glx_renderer will destroy textures and resources and complete the asynchronous
> stop. This does not happen because we run the main loop the same message loop
as
> glx renderer, in order to do this correctly, we need to:
> 
> 1. Create a new thread called glx_thread, pass the message loop of this thread
> to the renderer.
> 
> 2. Host all X calls on this glx_thread.
> 
> 3. Check for X events on this glx_thread. If shutdown is needed, post a task
to
> main message loop to call pipeline->Stop(). And when Stop() returns we can do
> XDestroyWindow() on this glx_thread, we simply run message->Quit().
> 
> 4. Implement asynchronous stop in the renderers.

http://codereview.chromium.org/2781010/diff/1/4#newcode145
media/tools/player_x11/player_x11.cc:145: return;
The message_loop only handles paint action here. As long as we wait for pipeline
fully stopped, there is no leftover.

On 2010/06/11 18:38:02, jiesun wrote:
> I believe a lot of logic ( include clean up task ) is done in message loop, (
> because of threading issue). it may not wise to QuitNow() to leave the mess
> behind.
> how do you think. could the task be stopped from render? or could you explain
> more? are Render stopped asynchronously or it had to stop after decoder
stopped?

[jiesun, 2010-06-14 23:11:01]

LGTM. 

for FillBufferDone and FreeOutputBuffer concern, you could address it in next
patch.

thanks

[Alpha Left Google, 2010-06-15 00:39:34]

I know this patch is committed, I hope we don't overlook the problem.

Please take a look in gles_video_renderer.cc.

InitializeGles() is called lazily in Paint(). Which means initialization of GLES
and textures are all done on the main thread. Which is on the message loop in
player_x11.cc. Let's call it the X thread.

However destruction of the EGL context and textures are done in OnStop(). Which
on the video renderer thread or the pipeline thread. Context switching in GLES
is not safe. The best place to do destruction is on the X thread which is always
safe. This is not possible because X thread is blocked on Pipeline::Stop(), so
you can't post a task onto X thread when stopping to do cleanup. The right thing
to do is to separate the X thread and main thread. Let the X thread to *only*
host X calls and all GLES calls. And we shutdown pipeline on the main thread,
which is on the main message loop.