Fix GonkVideoDecoderManager shutdown during initialization

RESOLVED FIXED in Firefox 44

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla44
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
GonkVideoDecoderManager seems not handle shutdown during initialization(video codec is not reserved).
(Assignee)

Comment 1

2 years ago
Created attachment 8668001 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization
(Assignee)

Updated

2 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

2 years ago
Attachment #8668001 - Flags: review?(bwu)
I think you are trying to fix the same problem in bug 1204622 as well.
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.
Attachment #8668001 - Flags: review?(ayang)
(Assignee)

Comment 4

2 years ago
(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 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?
Attachment #8668001 - Flags: review?(ayang)
(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.
OS: Unspecified → Gonk (Firefox OS)
Hardware: Unspecified → ARM
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.
Attachment #8668001 - Flags: review?(bwu) → review+
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Updated

2 years ago
See Also: → bug 1214997
(Assignee)

Comment 10

2 years ago
Created attachment 8674074 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization

Rebased.
Attachment #8668001 - Attachment is obsolete: true
Attachment #8674074 - Flags: review+
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c24bbbf69406
(Assignee)

Comment 12

2 years ago
Created attachment 8674628 [details] [diff] [review]
patch - Fix GonkVideoDecoderManager shutdown during initialization

Fix build failure on KK.
Attachment #8674074 - Attachment is obsolete: true
Attachment #8674628 - Flags: review+
(Assignee)

Comment 13

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

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6fd71be8de
Duplicate of this bug: 1207214
Duplicate of this bug: 1214502
https://hg.mozilla.org/mozilla-central/rev/cd6fd71be8de
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.