1210045 - Fix GonkVideoDecoderManager shutdown during initialization

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Sotaro Ikeda [:sotaro]]

GonkVideoDecoderManager seems not handle shutdown during initialization(video codec is not reserved).

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix GonkVideoDecoderManager shutdown during initialization

Comment 2

[Blake Wu [:bwu][:blakewu]]

I think you are trying to fix the same problem in bug 1204622 as well.

Comment 3

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8668001 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization

Add Alfredo to review this as well since he is working on bug 1204622.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #3)
> Comment on attachment 8668001 [details] [diff] [review]
> patch - Fix GonkVideoDecoderManager shutdown during initialization
> 
> Add Alfredo to review this as well since he is working on bug 1204622.

I saw bug 1204622. It seems not ensure thread safety in all cases.

Comment 5

[Alfredo Yang (:alfredo)]

Comment on attachment 8668001 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization

Review of attachment 8668001 [details] [diff] [review]:
-----------------------------------------------------------------

It is good idea to use promise for resource manager here. IMHO it'd be better to use promise in MediaSystemResourceClient, not just GonkVideoDecoderManager.
For example, refactory MediaSystemResourceClient->Acquire() to return a promise and then eliminating the callback MediaSystemResourceReservationListener.
So we can remove a lot of code like GonkVideoDecoderManager::VideoResourceListener and simplify the codes in MediaCodecProxy.

What do you think?

Comment 6

[Blake Wu [:bwu][:blakewu]]

(In reply to Alfredo Yang (:alfredo) from comment #5)
> Comment on attachment 8668001 [details] [diff] [review]
> patch - Fix GonkVideoDecoderManager shutdown during initialization
> 
> Review of attachment 8668001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It is good idea to use promise for resource manager here. IMHO it'd be
> better to use promise in MediaSystemResourceClient, not just
> GonkVideoDecoderManager.
> For example, refactory MediaSystemResourceClient->Acquire() to return a
> promise and then eliminating the callback
> MediaSystemResourceReservationListener.
IIRC, main reasons are in bug 1112219 comment 42.

Comment 7

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8668001 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization

Review of attachment 8668001 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!
Sorry to the late review.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Alfredo Yang (:alfredo) from comment #5)
> Comment on attachment 8668001 [details] [diff] [review]
> patch - Fix GonkVideoDecoderManager shutdown during initialization
> 
> Review of attachment 8668001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It is good idea to use promise for resource manager here. IMHO it'd be
> better to use promise in MediaSystemResourceClient, not just
> GonkVideoDecoderManager.
> For example, refactory MediaSystemResourceClient->Acquire() to return a
> promise and then eliminating the callback
> MediaSystemResourceReservationListener.
> So we can remove a lot of code like
> GonkVideoDecoderManager::VideoResourceListener and simplify the codes in
> MediaCodecProxy.
> 
> What do you think?

Thanks for the comment. I am going to think about it in a different bug.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #6)
> > 
> > It is good idea to use promise for resource manager here. IMHO it'd be
> > better to use promise in MediaSystemResourceClient, not just
> > GonkVideoDecoderManager.
> > For example, refactory MediaSystemResourceClient->Acquire() to return a
> > promise and then eliminating the callback
> > MediaSystemResourceReservationListener.
> IIRC, main reasons are in bug 1112219 comment 42.

The situation seems to be changed since the comment. I am going to think about it in a different bug.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix GonkVideoDecoderManager shutdown during initialization

Rebased.

Comment 11

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24bbbf69406

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix GonkVideoDecoderManager shutdown during initialization

Fix build failure on KK.

Comment 13

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea2b5d126d2

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6fd71be8de

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd6fd71be8de