If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[A/V] H/W decoder cannot be shared between applications/tasks

VERIFIED FIXED in Firefox 24, Firefox OS v1.1hd

Status

()

Core
Audio/Video
P1
major
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: leo.bugzilla.gecko, Assigned: sotaro)

Tracking

unspecified
1.1 QE2 (6jun)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: c=video , MiniWW, [TD-26566])

Attachments

(2 attachments, 64 obsolete attachments)

80.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
83.33 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Pre-condition : put many video files in SD card
Start video application with new inserted SD card -> home button -> (thumbnail is retrieving in background) -> start music or browser

[1] music application cannot use H/W decoder 
 -> This can be consume more power and overall performance is decreased
[2] browser cannot play any streaming
 -> Because browser cannot use h/W codec for video decoding
 -> If browser start streaming once, video application cannot listup any thumbnail while playing
(Reporter)

Updated

4 years ago
blocking-b2g: --- → leo?
Sotaro - this looks like a bigger issue of process sharing with the hardware decoder, can you weigh in on whether this would be considered a new feature or is part of any existing work?
Flags: needinfo?(sotaro.ikeda.g)
Summary: [A/V] Background thumbnail retrieving affect to ohter application → [A/V] H/W decoder cannot be shared between applications/tasks
(Assignee)

Comment 2

4 years ago
This is a new feature. I am going to discuss about in web rendering work week in Taipei next week(5/20-24).
Flags: needinfo?(sotaro.ikeda.g)
(Reporter)

Updated

4 years ago
Target Milestone: --- → 1.1 QE2
(Reporter)

Comment 3

4 years ago
Bug 871913 is also related to this issue.
(Reporter)

Updated

4 years ago
Priority: -- → P1
(In reply to leo.bugzilla.gecko from comment #0)
> Pre-condition : put many video files in SD card
> Start video application with new inserted SD card -> home button ->
> (thumbnail is retrieving in background) -> start music or browser

The video app stops scanning videos as soon as possible after it goes to the background. The hardware decoder should be free for other apps to use shortly after the video app is hidden.

> 
> [1] music application cannot use H/W decoder 
>  -> This can be consume more power and overall performance is decreased

It should be able too. How can we tell whether the music app is using the hardware decoder or not?  Could you be more specific about how to reproduce this?

> [2] browser cannot play any streaming
>  -> Because browser cannot use h/W codec for video decoding

Are you sure? Can you give more specific steps to reproduce?

>  -> If browser start streaming once, video application cannot listup any
> thumbnail while playing

The video app no longer even tries to do that.

It sounds to me like this bug should have been fixed by bug 856542, which was uplifted to v1-train on 5/3.

Leo: would you please check again that this is still something you can reproduce?  And if so, would you provide more specific steps to reproduce it?
Flags: needinfo?(leo.bugzilla.gecko)
(In reply to leo.bugzilla.gecko from comment #0)
> [1] music application cannot use H/W decoder 
>  -> This can be consume more power and overall performance is decreased

I don't know what will happen if we're playing music in the background (using the hw decoder) and the user starts the video app.  Will gecko automatically switch the music playback to a software decoder?  Also, what kind of music files does the hardware decoder get used for?  .mp3? .m4a?  Sotaro, do you know the answers?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 6

4 years ago
Currently there are no mechanism to share hw audio codec between apps. .mp3? .m4a both can use it. I think, current Firefox OS phones can instantiate only one hw audio codec at a time.
It is used to reduce power consumption during audio playback. So, basically it should be used only by Music app. We need to add it to gecko. Next week, I am going to discuss about it in web rendering work week in Taipei.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)

Updated

4 years ago
Whiteboard: c=video
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 7

4 years ago
(In reply to David Flanagan [:djf] from comment #5)
> I don't know what will happen if we're playing music in the background
> (using the hw decoder) and the user starts the video app.  Will gecko
> automatically switch the music playback to a software decoder?  Also, what
> kind of music files does the hardware decoder get used for?  .mp3? .m4a? 
> Sotaro, do you know the answers?

I think Sotaro's answer has all of answers.
Flags: needinfo?(leo.bugzilla.gecko)

Comment 8

4 years ago
To decide on blocking, we need to understand if this is a 1.1 regression from 1.0.1 behavior. Otherwise this is a new feature.
(Assignee)

Comment 9

4 years ago
In v1.0.1, hw codec usage is restricted only to some certified apps. And hw codec resources are controlled by the application(gaia side).
From v1.1, any application and any web content could use hw codec, then gecko needs to handle hardware codec usage contention. So, it is a new feature.
(Assignee)

Updated

4 years ago
Blocks: 831747
(Assignee)

Comment 10

4 years ago
I am going to implement simple resource manager in android mediaserver. In Firefox OS, b2g process basically controls resource allocations. But if b2g process does this, app's performance could degrade significantly. The mediaserver actually allocate hw codec resources. Therefore the mediaserver is a correct place to control it.
Sotaro: if you fix this bug, does it mean that we'll be able to take all of the resource sharing hacks out of gaia?
(Assignee)

Comment 12

4 years ago
djf, When it is implemented, gecko can prevent video allocation failure come from resource limitation. It becomes to wait until resource allocation.
It can fix the problems of inter app conflict. But it does not fix all conflicts within application. If the resource is still used, the latter in the same app can not get hw video codec.
(Assignee)

Comment 13

4 years ago
Created attachment 751540 [details]
Plan of changing nsBuiltinDecoder's state
(Assignee)

Comment 14

4 years ago
Created attachment 751541 [details]
Plan of changing nsBuiltinDecoderStateMachine's state
(Assignee)

Comment 15

4 years ago
The current state diagrams of nsBuiltinDecoderStateMachine and nsBuiltinDecoder on v1.01 is at https://github.com/sotaroikeda/firefox-diagrams/wiki/Firefox-Diagrams
(Assignee)

Comment 16

4 years ago
Created attachment 751548 [details]
design diagram of MeiaResourceManager

Comment 17

4 years ago
Sotaro, will you fall back on the software decoder when the hardware decoder is in use?
(Assignee)

Comment 18

4 years ago
For v1.1, I will not fall back on the software video codec by following reasons. It is going to wait until got hw video codec.

- Sw video codec was disabled by Bug 834150. Enabling sw video codec could regress the bug.
- H.264 sw codec supports only base line profile.
- sw video codec's playback performance is very bad compared to hw codec.
   It could not provide product level performance and quality for video playback.
- Event handling in applications are asynchronous between applications.
   If there are fallback path to sw codec. Normal video playback could downgrade to sw codec depands on the timing.
(Assignee)

Comment 19

4 years ago
In audio case, it is already fallbacked to sw codec.
(Assignee)

Comment 20

4 years ago
When video tag loads a video codec, the codecs should support both playback and drawing to canvas. But there is a motivation to use sw video codec for thumbnail generation.
Today, I discussed with :doublec and :cpearce and agreed to create API for generating thumbnails for HTML videos in future Firefox OS Bug 873959.
Triage: Sotaro (or David), can you confirm what exactly the user impact of this bug currently is?

It sounds as though multiple videos can not be played at the same time but audio can play by falling back to the software codec, at the cost of performance. Another issue might be that if a video is playing in another app, the video app would have to wait for the hardware codec to become available in order to generate thumbnails.

This certainly sounds like something that could be improved, but is the current user impact bad enough to block the release on?
Flags: needinfo?(sotaro.ikeda.g)
Blocks a blocker.
blocking-b2g: leo? → leo+
(Assignee)

Comment 23

4 years ago
Video playback/thumbnail generation fails even when gaia app instantiate only one video at a time. Because hw codec's free in gecko side happens asynchronously from gaia side.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 24

4 years ago
Created attachment 753261 [details] [diff] [review]
Part 1 - add MediaResourceManagerService to mediaserver
(Assignee)

Comment 25

4 years ago
Created attachment 753262 [details] [diff] [review]
Part 2 - implement MediaResourceManagerService
(Assignee)

Comment 26

4 years ago
Created attachment 753263 [details] [diff] [review]
Part 3 - use MediaResourceManagerService in gecko media
(Assignee)

Comment 27

4 years ago
Created attachment 753266 [details] [diff] [review]
Part 3 v2 - use MediaResourceManagerService in gecko media

fix nits.
Attachment #753263 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
Complete MediaResourceManagerService's basic capabilities implementation. Next, I am going to implement DORMANT state.
(Assignee)

Comment 29

4 years ago
Created attachment 753329 [details] [diff] [review]
Part 3 v3 - use MediaResourceManagerService in gecko media

Add OMXCodecProxy.cpp and OMXCodecProxy.h.
Attachment #753266 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
Created attachment 753611 [details] [diff] [review]
Part 3 v4 - use MediaResourceManagerService in gecko media

fix nits bugs.
Attachment #753329 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Created attachment 754665 [details] [diff] [review]
Part 3 v5 - use MediaResourceManagerService in gecko media

Add Dormant mode.
Attachment #753611 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
attachment 754665 [details] [diff] [review] has following problem. I am not sure what is the reason of this problem.
- nsHTMLMediaElement::NotifyOwnerDocumentActivityChanged() is called only twice.
  After that, the function was not called when app/web page change between forground/backgoround.

Updated

4 years ago
Whiteboard: c=video → c=video , MiniWW
(Assignee)

Comment 33

4 years ago
Created attachment 755115 [details] [diff] [review]
Part 3 v6 - use MediaResourceManagerService in gecko media

- fix bugs in nsBuiltinDecoder's state handling
Attachment #754665 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Current patches works well in normal use cases. But one problem is recognized.
 - often see green video frame during changing active videos

Updated

4 years ago
Duplicate of this bug: 871410

Updated

4 years ago
Duplicate of this bug: 831747
(Assignee)

Comment 37

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> Current patches works well in normal use cases. But one problem is
> recognized.
>  - often see green video frame during changing active videos

It seems the problem happens by codes around ShadowImageLayerOGL.
(Assignee)

Comment 38

4 years ago
I confirmed that the problem is in ShadowImageLayerOGL::RenderLayer(). The function does not care about the following case. I am going to create a new bug for it.
 - no graphic buffer for rendering. but try to render a texture.
   It happened on rendering after VideoFrameContainer::ClearCurrentFrame().
(Assignee)

Updated

4 years ago
Depends on: 877400
(Assignee)

Comment 39

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #38)
> I confirmed that the problem is in ShadowImageLayerOGL::RenderLayer(). The
> function does not care about the following case. I am going to create a new
> bug for it.
>  - no graphic buffer for rendering. but try to render a texture.
>    It happened on rendering after VideoFrameContainer::ClearCurrentFrame().

Created Bug 877400.

Updated

4 years ago
Whiteboard: c=video , MiniWW → c=video , MiniWW, [TD-26566]
(Assignee)

Comment 40

4 years ago
current MediaResourceManagerService do not need to be in mediaserver porocess. To make code management easier, try to move the MediaResourceManagerService in b2g process.
(Assignee)

Comment 41

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #40)
> current MediaResourceManagerService do not need to be in mediaserver
> porocess. To make code management easier, try to move the
> MediaResourceManagerService in b2g process.

It is decided by offline discussion with :mwu.
(Assignee)

Updated

4 years ago
Attachment #753261 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #753262 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #755115 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 756302 [details] [diff] [review]
Part 1a - instantiate MediaResourceManagerService
(Assignee)

Comment 43

4 years ago
Created attachment 756303 [details] [diff] [review]
Part 1b - change to /b2g/app/Makefile.in
(Assignee)

Comment 44

4 years ago
Created attachment 756304 [details] [diff] [review]
Part 2a - implement MediaResourceManagerService
(Assignee)

Comment 45

4 years ago
Created attachment 756306 [details] [diff] [review]
Part 2b for b2g18 - Makefile.in for mediaresourcemanager
(Assignee)

Comment 46

4 years ago
Created attachment 756309 [details] [diff] [review]
Part 3a for b2g18 - change to nsHTMLMediaElement
(Assignee)

Comment 47

4 years ago
Created attachment 756310 [details] [diff] [review]
Part 3b - change to content/media/
(Assignee)

Comment 48

4 years ago
Created attachment 756311 [details] [diff] [review]
Part 3c - change to /content/media/omx/
(Assignee)

Comment 49

4 years ago
Created attachment 756312 [details] [diff] [review]
Part 3d for b2g18 - change to Makefile.in for omx
(Assignee)

Updated

4 years ago
Attachment #756302 - Flags: review?(mwu)
(Assignee)

Updated

4 years ago
Attachment #756303 - Flags: review?(mwu)
(Assignee)

Updated

4 years ago
Attachment #756304 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #756306 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #756309 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #756310 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #756311 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #756312 - Flags: review?(chris.double)
(Assignee)

Comment 50

4 years ago
Created attachment 756387 [details] [diff] [review]
Part 3e for b2g18 - change to layout/build/Makefile.in
(Assignee)

Updated

4 years ago
Attachment #756387 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Blocks: 878183
(Assignee)

Comment 51

4 years ago
Created attachment 756901 [details] [diff] [review]
Part 3f for b2g18 - change to content/media/Makefile.in
(Assignee)

Updated

4 years ago
Attachment #756901 - Flags: review?(chris.double)
(Reporter)

Comment 52

4 years ago
After adapting this patch, I cannot see problem...
But It seems like that youtube got another problem.

it's lagged so much..
(Reporter)

Comment 53

4 years ago
It occurred more frequently after patching Bug 870564
(Assignee)

Comment 54

4 years ago
(In reply to leo.bugzilla.gecko from comment #52)
> After adapting this patch, I cannot see problem...
> But It seems like that youtube got another problem.
> 
> it's lagged so much..

Can you explain about the lag more precisely?
The patches does not change how video decoding is done and playback, just change the timing of allocation of codecs.
(Reporter)

Comment 55

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #54)
> Can you explain about the lag more precisely?
> The patches does not change how video decoding is done and playback, just
> change the timing of allocation of codecs.

I know...so that,s weird.
Streaming jittered after adapting your patch.

I tested url which is in bug 870564.

what,s the so many read logs from proxy??
(Assignee)

Comment 56

4 years ago
(In reply to leo.bugzilla.gecko from comment #55)
> (In reply to Sotaro Ikeda [:sotaro] from comment #54)
> > Can you explain about the lag more precisely?
> > The patches does not change how video decoding is done and playback, just
> > change the timing of allocation of codecs.
> 
> I know...so that,s weird.
> Streaming jittered after adapting your patch.

My current assumtion is in Bug 877461 comment #39. The patches in this bug stimuates this problem, I am assuming.
(Assignee)

Comment 57

4 years ago
(In reply to leo.bugzilla.gecko from comment #55)
> 
> what,s the so many read logs from proxy??

I forgot to disable android style logs. I am going to update patches soon.

>#define LOG_NDEBUG 0
(Assignee)

Comment 58

4 years ago
Created attachment 757163 [details] [diff] [review]
Part 2a v2 - implement MediaResourceManagerService

disable logout.
Attachment #756304 - Attachment is obsolete: true
Attachment #756304 - Flags: review?(chris.double)
(Assignee)

Comment 59

4 years ago
Created attachment 757164 [details] [diff] [review]
Part 3c v2 - change to /content/media/omx/

disable logout
Attachment #756311 - Attachment is obsolete: true
Attachment #756311 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #757163 - Flags: review?(chris.double)
(Assignee)

Updated

4 years ago
Attachment #757164 - Flags: review?(chris.double)
(Assignee)

Comment 60

4 years ago
(In reply to leo.bugzilla.gecko from comment #55)
> what,s the so many read logs from proxy??

I updated the patch to disable the log. Can you confirm it?
(Reporter)

Comment 61

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #60)
> I updated the patch to disable the log. Can you confirm it?

Yes, it's removed.

I am checking one thing...
I think...Android style logs in Gecko side increase CPU usage.
When you log are included in Gecko side.

printf occupies 10~20% cpu.
After removing it...it's decreased..
Youtube fills find after removing android style logs.

I will check more and inform you again.

Updated

4 years ago
Attachment #756306 - Flags: review?(chris.double) → review+

Updated

4 years ago
Attachment #756309 - Flags: review?(chris.double) → review+

Comment 62

4 years ago
Comment on attachment 757163 [details] [diff] [review]
Part 2a v2 - implement MediaResourceManagerService

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

I notice most of the classes use Android naming conventions and styles instead of the Mozilla ones. I've not asked for this to be changed as it's mostly Android level code and it'll be more familiar to those who understand Android to stick with the Android conventions.

r+ with the changes identified. Mostly about comments being needed in places.

::: content/media/omx/mediaresourcemanager/IMediaResourceManagerDeathNotifier.cpp
@@ +12,5 @@
> +** distributed under the License is distributed on an "AS IS" BASIS,
> +** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +** See the License for the specific language governing permissions and
> +** limitations under the License.
> +*/

Why not the MPL?

@@ +46,5 @@
> +            if (binder != 0) {
> +                break;
> +             }
> +             LOGW("Media resource manager service not published, waiting...");
> +             usleep(500000); // 0.5 s

Where does this number come from? Document why it was chosen.

::: content/media/omx/mediaresourcemanager/IMediaResourceManagerDeathNotifier.h
@@ +12,5 @@
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

Why does this have a different license?

@@ +24,5 @@
> +#include "IMediaResourceManagerService.h"
> +
> +namespace android {
> +
> +class IMediaResourceManagerDeathNotifier: virtual public RefBase

Why virtual inheritance?

::: content/media/omx/mediaresourcemanager/IMediaResourceManagerService.cpp
@@ +16,5 @@
> +#include "IMediaResourceManagerService.h"
> +
> +namespace android {
> +
> +enum {

Document what these are.

::: content/media/omx/mediaresourcemanager/IMediaResourceManagerService.h
@@ +21,5 @@
> +{
> +public:
> +    DECLARE_META_INTERFACE(MediaResourceManagerService);
> +
> +    // register a current  for audio output change notifications

Is current the wrong word?

::: content/media/omx/mediaresourcemanager/MediaResourceManagerClient.cpp
@@ +15,5 @@
> +
> +MediaResourceManagerClient::MediaResourceManagerClient(const wp<EventListener>& listener)
> +  : mEventListener(listener)
> +{
> +  LOGV("MediaResourceManagerClient()");

No need for these LOG's I think.

::: content/media/omx/mediaresourcemanager/MediaResourceManagerClient.h
@@ +17,5 @@
> +{
> +public:
> +  // Enumeration for the valid decoding states
> +  enum State {
> +    CLIENT_STATE_WAIT_FOR_REASOURCE,

REASOURCE is spelt wrong.

@@ +28,5 @@
> +    HW_AUDIO_DECODER,
> +    HW_CAMERA
> +  };
> +
> +  struct EventListener : public virtual RefBase {

Why is virtual inheritance needed?

::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp
@@ +23,5 @@
> +
> +MediaResourceManagerService::MediaResourceManagerService()
> +  : mVideoDecoderCount(VIDEO_DECODER_COUNT)
> +{
> +  LOGV("MediaResourceManagerService()");

I don't think we need to log these.

::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.h
@@ +17,5 @@
> +#include "IMediaResourceManagerService.h"
> +
> +namespace android {
> +
> +class MediaResourceManagerService: public BnMediaResourceManagerService,

Can we have a comment explaining what this class does/what it's for. Or in one of the files have a comment explaining the overall architecture like nsBuiltinDecoder.h does for the standard media stuff.

@@ +21,5 @@
> +class MediaResourceManagerService: public BnMediaResourceManagerService,
> +                                   public IBinder::DeathRecipient
> +{
> +public:
> +  enum { VIDEO_DECODER_COUNT = 1 };

Comment explaining what this is for. Is it the maximum number of hardware decoders available?

@@ +43,5 @@
> +  void onMessageReceived(const sp<AMessage> &msg);
> +
> +protected:
> +  MediaResourceManagerService();
> +  ~MediaResourceManagerService();

Should this be virtual?

@@ +46,5 @@
> +  MediaResourceManagerService();
> +  ~MediaResourceManagerService();
> +
> +protected:
> +  struct ResourceSlot {

Explain in a comment what this is for.

@@ +56,5 @@
> +
> +  void cancelClientLocked(const sp<IBinder>& binder);
> +
> +  ResourceSlot mVideoDecoderSlots[VIDEO_DECODER_COUNT];
> +  int mVideoDecoderCount;

Comment explaining what this is for, and threading requirements for accessing it.

@@ +58,5 @@
> +
> +  ResourceSlot mVideoDecoderSlots[VIDEO_DECODER_COUNT];
> +  int mVideoDecoderCount;
> +
> +  Mutex mLock;

Comment explaining what the lock protects. ie. what methods/variables can and cannot be accessed with/without the lock.
Attachment #757163 - Flags: review?(chris.double) → review+

Comment 63

4 years ago
Comment on attachment 756310 [details] [diff] [review]
Part 3b - change to content/media/

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

Document the new states in the nsMediaDecoder.h comment.

::: content/media/nsBuiltinDecoder.cpp
@@ +42,5 @@
> +    // no change to dormant state
> +    return;
> +  }
> +
> +  if(aDormant == true) {

if (aDormant) {

::: content/media/nsBuiltinDecoder.h
@@ +836,5 @@
> +  // True if metadata is already loaded in the past.
> +  bool mIsMetadataLoaded;
> +
> +  // True if this decoder is in dormant state.
> +  // Could be true only when PlayState is PLAY_STATE_LOADING.

'Could' should be 'Should'?

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +511,5 @@
> +        NS_ASSERTION(mState == DECODER_STATE_SHUTDOWN,
> +                     "Should be in shutdown state if metadata loading fails.");
> +        LOG(PR_LOG_DEBUG, ("Decode metadata failed, shutting down decode thread"));
> +      }
> +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {

RESOURCES spelt wrong.

@@ +514,5 @@
> +      }
> +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {
> +      mDecoder->GetReentrantMonitor().Wait();
> +
> +      if (mReader->IsWaitingMediaResources() != true) {

Don't check for 'true' explicitly. Just use:

if (!mReader->IsWaitingMediaResources()) {

@@ +515,5 @@
> +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {
> +      mDecoder->GetReentrantMonitor().Wait();
> +
> +      if (mReader->IsWaitingMediaResources() != true) {
> +        // change state to DECODER_STATE_WAIT_FOR_REASOURCES

Spelling.

@@ +1451,5 @@
> +  if (!mReader) {
> +    return;
> +  }
> +
> +  if (aDormant == true) {

if (aDormant)

@@ +1831,5 @@
>    {
>      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>      res = mReader->ReadMetadata(&info, &tags);
>    }
> +  if (NS_SUCCEEDED(res) && (mState == DECODER_STATE_DECODING_METADATA) && (mReader->IsWaitingMediaResources() == true)) {

(mReader->IsWaitingMediaResources()) {

@@ +1832,5 @@
>      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>      res = mReader->ReadMetadata(&info, &tags);
>    }
> +  if (NS_SUCCEEDED(res) && (mState == DECODER_STATE_DECODING_METADATA) && (mReader->IsWaitingMediaResources() == true)) {
> +    // change state to DECODER_STATE_WAIT_FOR_REASOURCES

Spelling.

@@ +2089,5 @@
>        StopDecodeThread();
>        // Now that those threads are stopped, there's no possibility of
>        // mPendingWakeDecoder being needed again. Revoke it.
>        mPendingWakeDecoder = nullptr;
> +      // release media resources

No need for this comment as it duplicates the name of the method.

@@ +2132,5 @@
>      case DECODER_STATE_DECODING_METADATA: {
>        // Ensure we have a decode thread to decode metadata.
>        return ScheduleDecodeThread();
>      }
>    

Remove whitespace.

::: content/media/nsBuiltinDecoderStateMachine.h
@@ +118,3 @@
>    virtual nsresult Init(nsDecoderStateMachine* aCloneDonor);
>    State GetState()
>    { 

Remove added white space.

@@ +118,5 @@
>    virtual nsresult Init(nsDecoderStateMachine* aCloneDonor);
>    State GetState()
>    { 
>      mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
>      return mState; 

Remove added white space.

::: content/media/nsMediaDecoder.h
@@ +105,5 @@
>  
>    // Return true if the stream is infinite (see SetInfinite).
>    virtual bool IsInfinite() = 0;
>  
> +  // Set/Unset dormant state if necessary

Explain when it may be necessary and what dormant state is.
Attachment #756310 - Flags: review?(chris.double) → review+

Updated

4 years ago
Attachment #756312 - Flags: review?(chris.double) → review+

Comment 64

4 years ago
Comment on attachment 756387 [details] [diff] [review]
Part 3e for b2g18 - change to layout/build/Makefile.in

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

This will need to be reviewed by a layout/toolkit/build peer.
Attachment #756387 - Flags: review?(chris.double)

Updated

4 years ago
Attachment #756901 - Flags: review?(chris.double) → review+

Updated

4 years ago
Attachment #756303 - Flags: review?(mwu) → review+

Comment 65

4 years ago
Comment on attachment 757164 [details] [diff] [review]
Part 3c v2 - change to /content/media/omx/

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

::: content/media/omx/OMXCodecProxy.cpp
@@ +27,5 @@
> +        const char *matchComponentName,
> +        uint32_t flags,
> +        const sp<ANativeWindow> &nativeWindow)
> +{
> +  LOGV("OMXCodecProxy::Create()");

I don't think we need to log these.

@@ +71,5 @@
> +  if (mOMXCodec.get()) {
> +    wp<MediaSource> tmp = mOMXCodec;
> +    mOMXCodec.clear();
> +    while (tmp.promote() != NULL) {
> +        usleep(1000);

Document why this number was chosen.

@@ +177,5 @@
> +          mOMXCodec->getFormat()->findInt32(kKeyHeight, &height) &&
> +          width * height <= maxWidth * maxHeight)) {
> +      //printf_stderr("Failed to get video size, or it was too large for HW decoder (<w=%d, h=%d> but <maxW=%d, maxH=%d>)",
> +      //              width, height, maxWidth, maxHeight);
> +      // TODO

What is the TODO here for?

@@ +185,5 @@
> +    }
> +
> +    if (mOMXCodec->start() != OK) {
> +      //NS_WARNING("Couldn't start OMX video source");
> +      // TODO

What is the TODO here for?

::: content/media/omx/OMXCodecProxy.h
@@ +23,5 @@
> +class OMXCodecProxy : public MediaSource,
> +                      public MediaResourceManagerClient::EventListener
> +{
> +public:
> +  struct EventListener : public virtual RefBase {

Why virtual inheritance?

::: content/media/omx/OmxDecoder.cpp
@@ +254,5 @@
> +  }
> +
> +  //check if video is waiting resources
> +  if (mVideoSource.get()) {
> +    if (mVideoSource->IsWaitingResources() == true) {

if (mVideoSource->IsWaitingResources()) {

@@ +264,2 @@
>    int64_t totalDurationUs = 0;
> +  int64_t durationUs;

initialize to 0.

@@ +355,5 @@
> +    } else {
> +      sp<OMXCodecProxy::EventListener> listener = this;
> +      mVideoSource->setEventListener(listener);
> +      mVideoSource->requestResource();
> +    }    

Remove whitespace.

::: content/media/omx/nsMediaOmxReader.cpp
@@ +74,5 @@
> +  if (!mOmxDecoder->TryLoad()) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (IsWaitingMediaResources() == true) {

if (IsWaitingMediaResources()) {
Attachment #757164 - Flags: review?(chris.double) → review+

Comment 66

4 years ago
Comment on attachment 756302 [details] [diff] [review]
Part 1a - instantiate MediaResourceManagerService

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

::: b2g/app/nsBrowserApp.cpp
@@ +43,5 @@
> +# include <utils/String16.h>
> +
> +# include "MediaResourceManagerService.h"
> +
> +int waitBeforeAdding(const android::String16& serviceName ) {

The whitespace in this function needs cleanup.

@@ +59,5 @@
> + return -1;
> +}
> +
> +// Wait until service manager is started
> +void 

leading whitespace

@@ +216,5 @@
> +  // Wait until service manager is started
> +  waitServiceManager();
> +  waitBeforeAdding( android::String16("media.resource_manager") );
> +  // Instantiate and register MediaResourceManager
> +  android::MediaResourceManagerService::instantiate();

Not a fan of instantiating it here - it would be nicer to initialize this closer to its user in the media code, and also avoid slowing down startup. Is there another location you can do this?
(Assignee)

Comment 67

4 years ago
Comment on attachment 756387 [details] [diff] [review]
Part 3e for b2g18 - change to layout/build/Makefile.in

roc, can you review the patch?
Attachment #756387 - Flags: review?(roc)
(Assignee)

Comment 68

4 years ago
(In reply to Chris Double (:doublec) from comment #62)
> Comment on attachment 757163 [details] [diff] [review]
> Part 2a v2 - implement MediaResourceManagerService
> 
> Review of attachment 757163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I notice most of the classes use Android naming conventions and styles
> instead of the Mozilla ones. I've not asked for this to be changed as it's
> mostly Android level code and it'll be more familiar to those who understand
> Android to stick with the Android conventions.
> 
> r+ with the changes identified. Mostly about comments being needed in places.
> 
> :::
> content/media/omx/mediaresourcemanager/IMediaResourceManagerDeathNotifier.cpp
> @@ +12,5 @@
> > +** distributed under the License is distributed on an "AS IS" BASIS,
> > +** WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > +** See the License for the specific language governing permissions and
> > +** limitations under the License.
> > +*/
> 
> Why not the MPL?

Almost code is borrowed from android's IMediaDeathNotifier.

> @@ +46,5 @@
> > +            if (binder != 0) {
> > +                break;
> > +             }
> > +             LOGW("Media resource manager service not published, waiting...");
> > +             usleep(500000); // 0.5 s
> 
> Where does this number come from? Document why it was chosen.

Come from android's IMediaDeathNotifier.

> :::
> content/media/omx/mediaresourcemanager/IMediaResourceManagerDeathNotifier.h
> @@ +12,5 @@
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> 
> Why does this have a different license?

mistake. correct to apache2.0 in next patch

> 
> @@ +24,5 @@
> > +#include "IMediaResourceManagerService.h"
> > +
> > +namespace android {
> > +
> > +class IMediaResourceManagerDeathNotifier: virtual public RefBase
> 
> Why virtual inheritance?

Come from android's IMediaDeathNotifier. This class is expected to be used as multi ihheritance whth RefBase inherited class.

> 
> ::: content/media/omx/mediaresourcemanager/IMediaResourceManagerService.cpp
> @@ +16,5 @@
> > +#include "IMediaResourceManagerService.h"
> > +
> > +namespace android {
> > +
> > +enum {
> 
> Document what these are.

Update in next patch.

> 
> ::: content/media/omx/mediaresourcemanager/IMediaResourceManagerService.h
> @@ +21,5 @@
> > +{
> > +public:
> > +    DECLARE_META_INTERFACE(MediaResourceManagerService);
> > +
> > +    // register a current  for audio output change notifications
> 
> Is current the wrong word?

No, this comment is dust from old file. Fix in next version.

> 
> ::: content/media/omx/mediaresourcemanager/MediaResourceManagerClient.cpp
> @@ +15,5 @@
> > +
> > +MediaResourceManagerClient::MediaResourceManagerClient(const wp<EventListener>& listener)
> > +  : mEventListener(listener)
> > +{
> > +  LOGV("MediaResourceManagerClient()");
> 
> No need for these LOG's I think.

Remove in next version.

> 
> ::: content/media/omx/mediaresourcemanager/MediaResourceManagerClient.h
> @@ +17,5 @@
> > +{
> > +public:
> > +  // Enumeration for the valid decoding states
> > +  enum State {
> > +    CLIENT_STATE_WAIT_FOR_REASOURCE,
> 
> REASOURCE is spelt wrong.

Fix in next version.

> 
> @@ +28,5 @@
> > +    HW_AUDIO_DECODER,
> > +    HW_CAMERA
> > +  };
> > +
> > +  struct EventListener : public virtual RefBase {
> 
> Why is virtual inheritance needed?

It is android convention for Listener interface. This class is expected to be used as multi ihheritance whth RefBase inherited class.

> 
> ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp
> @@ +23,5 @@
> > +
> > +MediaResourceManagerService::MediaResourceManagerService()
> > +  : mVideoDecoderCount(VIDEO_DECODER_COUNT)
> > +{
> > +  LOGV("MediaResourceManagerService()");
> 
> I don't think we need to log these.

Remove in next version.

> ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.h
> @@ +17,5 @@
> > +#include "IMediaResourceManagerService.h"
> > +
> > +namespace android {
> > +
> > +class MediaResourceManagerService: public BnMediaResourceManagerService,
> 
> Can we have a comment explaining what this class does/what it's for. Or in
> one of the files have a comment explaining the overall architecture like
> nsBuiltinDecoder.h does for the standard media stuff.

Add comments in next version.

> 
> @@ +21,5 @@
> > +class MediaResourceManagerService: public BnMediaResourceManagerService,
> > +                                   public IBinder::DeathRecipient
> > +{
> > +public:
> > +  enum { VIDEO_DECODER_COUNT = 1 };
> 
> Comment explaining what this is for. Is it the maximum number of hardware
> decoders available?

Yes. It is current implementation. Add comment in next version.

> 
> @@ +43,5 @@
> > +  void onMessageReceived(const sp<AMessage> &msg);
> > +
> > +protected:
> > +  MediaResourceManagerService();
> > +  ~MediaResourceManagerService();
> 
> Should this be virtual?

Yes. Fix in next version.

> 
> @@ +46,5 @@
> > +  MediaResourceManagerService();
> > +  ~MediaResourceManagerService();
> > +
> > +protected:
> > +  struct ResourceSlot {
> 
> Explain in a comment what this is for.

Add comment in next version.

> 
> @@ +56,5 @@
> > +
> > +  void cancelClientLocked(const sp<IBinder>& binder);
> > +
> > +  ResourceSlot mVideoDecoderSlots[VIDEO_DECODER_COUNT];
> > +  int mVideoDecoderCount;
> 
> Comment explaining what this is for, and threading requirements for
> accessing it.

Add comment in next version.

> 
> @@ +58,5 @@
> > +
> > +  ResourceSlot mVideoDecoderSlots[VIDEO_DECODER_COUNT];
> > +  int mVideoDecoderCount;
> > +
> > +  Mutex mLock;
> 
> Comment explaining what the lock protects. ie. what methods/variables can
> and cannot be accessed with/without the lock.

Will add in next version.
Attachment #756387 - Flags: review?(roc) → review+

Updated

4 years ago
Flags: in-moztrap?
Comment on attachment 756310 [details] [diff] [review]
Part 3b - change to content/media/

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

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +2090,5 @@
>        // Now that those threads are stopped, there's no possibility of
>        // mPendingWakeDecoder being needed again. Revoke it.
>        mPendingWakeDecoder = nullptr;
> +      // release media resources
> +      mReader->ReleaseMediaResources();

I think you should release the decoder monitor while calling into the reader, as the ReleaseMediaResources() call could do anything. It could require something to run on the main thread, and the main or some other thread which could be blocked waiting for the decoder monitor in some other function, causing a deadlock.

So:

{
  ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
  mReader->ReleaseMediaResources();
}

@@ +2120,5 @@
> +      // Now that those threads are stopped, there's no possibility of
> +      // mPendingWakeDecoder being needed again. Revoke it.
> +      mPendingWakeDecoder = nullptr;
> +      // release media resources
> +      mReader->ReleaseMediaResources();

Again:

{
  ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
  mReader->ReleaseMediaResources();
}
(Assignee)

Comment 70

4 years ago
Created attachment 758692 [details] [diff] [review]
Part 2a v3 - implement MediaResourceManagerService

Apply comments.
Attachment #757163 - Attachment is obsolete: true
(Assignee)

Comment 71

4 years ago
(In reply to Michael Wu [:mwu] from comment #66)
> @@ +216,5 @@
> > +  // Wait until service manager is started
> > +  waitServiceManager();
> > +  waitBeforeAdding( android::String16("media.resource_manager") );
> > +  // Instantiate and register MediaResourceManager
> > +  android::MediaResourceManagerService::instantiate();
> 
> Not a fan of instantiating it here - it would be nicer to initialize this
> closer to its user in the media code, and also avoid slowing down startup.
> Is there another location you can do this?

There is no good place around media framework. This call takes 30ms. Is it OK to instantiate for v1.1?
(Assignee)

Updated

4 years ago
Flags: needinfo?(mwu)
(Assignee)

Updated

4 years ago
Flags: needinfo?(mwu)
(Assignee)

Comment 72

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #71)
> 
> There is no good place around media framework. This call takes 30ms. Is it
> OK to instantiate for v1.1?

By irc with mwu, for the time being, media resource manager will be instantiated in nsAppShell::Init().
(Assignee)

Comment 73

4 years ago
Created attachment 758732 [details] [diff] [review]
Part 1a v2 for b2g18 - instantiate MediaResourceManagerService

Change the place of resource manager instantiation from from manin() to nsAppShell::Init().
Attachment #756302 - Attachment is obsolete: true
Attachment #756302 - Flags: review?(mwu)
(Assignee)

Comment 74

4 years ago
Created attachment 758735 [details] [diff] [review]
Part 1b v2 - change to /widget/gonk/Makefile.in
Attachment #756303 - Attachment is obsolete: true
(Assignee)

Comment 75

4 years ago
Created attachment 758743 [details] [diff] [review]
Part 1b v3 for b2g18 - change to /widget/gonk/Makefile.in

remove unrelated part.
Attachment #758735 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #758732 - Flags: review?(mwu)
(Assignee)

Updated

4 years ago
Attachment #758743 - Flags: review?(mwu)
(Assignee)

Comment 76

4 years ago
Created attachment 758750 [details] [diff] [review]
Part 2a v4 for b2g18 - implement MediaResourceManagerService

Update MediaResourceManagerService::instantiate().
Carry "chris.double: review+"
Attachment #758750 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #758692 - Attachment is obsolete: true
(Assignee)

Comment 77

4 years ago
(In reply to Chris Double (:doublec) from comment #63)
> Comment on attachment 756310 [details] [diff] [review]
> Part 3b - change to content/media/
> 
> Review of attachment 756310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Document the new states in the nsMediaDecoder.h comment.
> 
> ::: content/media/nsBuiltinDecoder.cpp
> @@ +42,5 @@
> > +    // no change to dormant state
> > +    return;
> > +  }
> > +
> > +  if(aDormant == true) {
> 
> if (aDormant) {

Fix in next patch.

> 
> ::: content/media/nsBuiltinDecoder.h
> @@ +836,5 @@
> > +  // True if metadata is already loaded in the past.
> > +  bool mIsMetadataLoaded;
> > +
> > +  // True if this decoder is in dormant state.
> > +  // Could be true only when PlayState is PLAY_STATE_LOADING.
> 
> 'Could' should be 'Should'?

Yeah, should be "Should"

> 
> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> @@ +511,5 @@
> > +        NS_ASSERTION(mState == DECODER_STATE_SHUTDOWN,
> > +                     "Should be in shutdown state if metadata loading fails.");
> > +        LOG(PR_LOG_DEBUG, ("Decode metadata failed, shutting down decode thread"));
> > +      }
> > +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {
> 
> RESOURCES spelt wrong.

Fix in next patch.

> 
> @@ +514,5 @@
> > +      }
> > +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {
> > +      mDecoder->GetReentrantMonitor().Wait();
> > +
> > +      if (mReader->IsWaitingMediaResources() != true) {
> 
> Don't check for 'true' explicitly. Just use:

Fix in next patch.

> 
> if (!mReader->IsWaitingMediaResources()) {
> 
> @@ +515,5 @@
> > +    } else if (mState == DECODER_STATE_WAIT_FOR_REASOURCES) {
> > +      mDecoder->GetReentrantMonitor().Wait();
> > +
> > +      if (mReader->IsWaitingMediaResources() != true) {
> > +        // change state to DECODER_STATE_WAIT_FOR_REASOURCES
> 
> Spelling.

Fix in next patch.

> 
> @@ +1451,5 @@
> > +  if (!mReader) {
> > +    return;
> > +  }
> > +
> > +  if (aDormant == true) {
> 
> if (aDormant)

Fix in next patch.

> 
> @@ +1831,5 @@
> >    {
> >      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> >      res = mReader->ReadMetadata(&info, &tags);
> >    }
> > +  if (NS_SUCCEEDED(res) && (mState == DECODER_STATE_DECODING_METADATA) && (mReader->IsWaitingMediaResources() == true)) {
> 
> (mReader->IsWaitingMediaResources()) {

Fix in next patch.

> 
> @@ +1832,5 @@
> >      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> >      res = mReader->ReadMetadata(&info, &tags);
> >    }
> > +  if (NS_SUCCEEDED(res) && (mState == DECODER_STATE_DECODING_METADATA) && (mReader->IsWaitingMediaResources() == true)) {
> > +    // change state to DECODER_STATE_WAIT_FOR_REASOURCES
> 
> Spelling.

Fix in next patch.

> 
> @@ +2089,5 @@
> >        StopDecodeThread();
> >        // Now that those threads are stopped, there's no possibility of
> >        // mPendingWakeDecoder being needed again. Revoke it.
> >        mPendingWakeDecoder = nullptr;
> > +      // release media resources
> 
> No need for this comment as it duplicates the name of the method.

Fix in next patch.

> 
> @@ +2132,5 @@
> >      case DECODER_STATE_DECODING_METADATA: {
> >        // Ensure we have a decode thread to decode metadata.
> >        return ScheduleDecodeThread();
> >      }
> >    
> 
> Remove whitespace.

Fix in next patch.

> 
> ::: content/media/nsBuiltinDecoderStateMachine.h
> @@ +118,3 @@
> >    virtual nsresult Init(nsDecoderStateMachine* aCloneDonor);
> >    State GetState()
> >    { 
> 
> Remove added white space.

Fix in next patch.

> 
> @@ +118,5 @@
> >    virtual nsresult Init(nsDecoderStateMachine* aCloneDonor);
> >    State GetState()
> >    { 
> >      mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> >      return mState; 
> 
> Remove added white space.

Fix in next patch.

> 
> ::: content/media/nsMediaDecoder.h
> @@ +105,5 @@
> >  
> >    // Return true if the stream is infinite (see SetInfinite).
> >    virtual bool IsInfinite() = 0;
> >  
> > +  // Set/Unset dormant state if necessary
> 
> Explain when it may be necessary and what dormant state is.

Add comment in next patch.
(Assignee)

Comment 78

4 years ago
(In reply to Chris Pearce (:cpearce) from comment #69)
> Comment on attachment 756310 [details] [diff] [review]
> Part 3b - change to content/media/
> 
> Review of attachment 756310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/nsBuiltinDecoderStateMachine.cpp
> @@ +2090,5 @@
> >        // Now that those threads are stopped, there's no possibility of
> >        // mPendingWakeDecoder being needed again. Revoke it.
> >        mPendingWakeDecoder = nullptr;
> > +      // release media resources
> > +      mReader->ReleaseMediaResources();
> 
> I think you should release the decoder monitor while calling into the
> reader, as the ReleaseMediaResources() call could do anything. It could
> require something to run on the main thread, and the main or some other
> thread which could be blocked waiting for the decoder monitor in some other
> function, causing a deadlock.
> 
> So:
> 
> {
>   ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
>   mReader->ReleaseMediaResources();
> }

Thanks for the comment. Fix in next patch.
(Assignee)

Comment 79

4 years ago
Created attachment 758789 [details] [diff] [review]
Part 3b v2 for b2g18 - change to content/media/

Apply comment.
Attachment #756310 - Attachment is obsolete: true
(Assignee)

Comment 80

4 years ago
Comment on attachment 758789 [details] [diff] [review]
Part 3b v2 for b2g18 - change to content/media/

Carry "chris.double: review+".
Attachment #758789 - Flags: review+

Updated

4 years ago
Attachment #758743 - Flags: review?(mwu) → review+

Comment 81

4 years ago
Comment on attachment 758732 [details] [diff] [review]
Part 1a v2 for b2g18 - instantiate MediaResourceManagerService

This looks much simpler in nsAppShell.cpp. Thanks
Attachment #758732 - Flags: review?(mwu) → review+
(Assignee)

Comment 82

4 years ago
(In reply to Chris Double (:doublec) from comment #65)
> Comment on attachment 757164 [details] [diff] [review]
> Part 3c v2 - change to /content/media/omx/
> 
> Review of attachment 757164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecProxy.cpp
> @@ +27,5 @@
> > +        const char *matchComponentName,
> > +        uint32_t flags,
> > +        const sp<ANativeWindow> &nativeWindow)
> > +{
> > +  LOGV("OMXCodecProxy::Create()");
> 
> I don't think we need to log these.

Remove in next patch.

> 
> @@ +71,5 @@
> > +  if (mOMXCodec.get()) {
> > +    wp<MediaSource> tmp = mOMXCodec;
> > +    mOMXCodec.clear();
> > +    while (tmp.promote() != NULL) {
> > +        usleep(1000);
> 
> Document why this number was chosen.

It comes from stagefright's AwesomePlayer's implementation. Add comment in next patch.

> 
> @@ +177,5 @@
> > +          mOMXCodec->getFormat()->findInt32(kKeyHeight, &height) &&
> > +          width * height <= maxWidth * maxHeight)) {
> > +      //printf_stderr("Failed to get video size, or it was too large for HW decoder (<w=%d, h=%d> but <maxW=%d, maxH=%d>)",
> > +      //              width, height, maxWidth, maxHeight);
> > +      // TODO
> 
> What is the TODO here for?

It is about to enable the above log. Enable log in next patch.

> 
> @@ +185,5 @@
> > +    }
> > +
> > +    if (mOMXCodec->start() != OK) {
> > +      //NS_WARNING("Couldn't start OMX video source");
> > +      // TODO
> 
> What is the TODO here for?

It is about to enable the above log. Enable log in next patch.

> 
> ::: content/media/omx/OMXCodecProxy.h
> @@ +23,5 @@
> > +class OMXCodecProxy : public MediaSource,
> > +                      public MediaResourceManagerClient::EventListener
> > +{
> > +public:
> > +  struct EventListener : public virtual RefBase {
> 
> Why virtual inheritance?

It is android convention for Listener interface. This class is expected to be used as multi ihheritance whth RefBase inherited class.


> 
> ::: content/media/omx/OmxDecoder.cpp
> @@ +254,5 @@
> > +  }
> > +
> > +  //check if video is waiting resources
> > +  if (mVideoSource.get()) {
> > +    if (mVideoSource->IsWaitingResources() == true) {
> 
> if (mVideoSource->IsWaitingResources()) {

Fix in next patch.

> 
> @@ +264,2 @@
> >    int64_t totalDurationUs = 0;
> > +  int64_t durationUs;
> 
> initialize to 0.

Fix in next patch.

> 
> @@ +355,5 @@
> > +    } else {
> > +      sp<OMXCodecProxy::EventListener> listener = this;
> > +      mVideoSource->setEventListener(listener);
> > +      mVideoSource->requestResource();
> > +    }    
> 
> Remove whitespace.

Fix in next patch.

> 
> ::: content/media/omx/nsMediaOmxReader.cpp
> @@ +74,5 @@
> > +  if (!mOmxDecoder->TryLoad()) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  if (IsWaitingMediaResources() == true) {
> 
> if (IsWaitingMediaResources()) {

Fix in next patch.
(Assignee)

Comment 83

4 years ago
Created attachment 758813 [details] [diff] [review]
Part 3c v3 for b2g18 - change to /content/media/omx/

Apply comment. Carry "chris.double: review+".
Attachment #757164 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #758813 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #756306 - Attachment description: Part 2b - Makefile.in for mediaresourcemanager → Part 2b for b2g18 - Makefile.in for mediaresourcemanager
(Assignee)

Updated

4 years ago
Attachment #756309 - Attachment description: Part 3a - change to nsHTMLMediaElement → Part 3a for b2g18 - change to nsHTMLMediaElement
(Assignee)

Updated

4 years ago
Attachment #756312 - Attachment description: Part 3d - change to Makefile.in for omx → Part 3d for b2g18 - change to Makefile.in for omx
(Assignee)

Updated

4 years ago
Attachment #756387 - Attachment description: Part 3e - change to layout/build/Makefile.in → Part 3e for b2g18 - change to layout/build/Makefile.in
(Assignee)

Updated

4 years ago
Attachment #756901 - Attachment description: Part 3f - change to content/media/Makefile.in → Part 3f for b2g18 - change to content/media/Makefile.in
(Assignee)

Updated

4 years ago
Attachment #758732 - Attachment description: Part 1a v2 - instantiate MediaResourceManagerService → Part 1a for b2g18 v2 - instantiate MediaResourceManagerService
(Assignee)

Updated

4 years ago
Attachment #758743 - Attachment description: Part 1b v3 - change to /widget/gonk/Makefile.in → Part 1b v3 for b2g18 - change to /widget/gonk/Makefile.in
(Assignee)

Updated

4 years ago
Attachment #758732 - Attachment description: Part 1a for b2g18 v2 - instantiate MediaResourceManagerService → Part 1a v2 for b2g18 - instantiate MediaResourceManagerService
(Assignee)

Updated

4 years ago
Attachment #758750 - Attachment description: Part 2a v4 - implement MediaResourceManagerService → Part 2a v4 for b2g18 - implement MediaResourceManagerService
(Assignee)

Updated

4 years ago
Attachment #758789 - Attachment description: Part 3b v2 - change to content/media/ → Part 3b v2 for b2g18 - change to content/media/
(Assignee)

Updated

4 years ago
Attachment #758813 - Attachment description: Part 3c v3 - change to /content/media/omx/ → Part 3c v3 for b2g18 - change to /content/media/omx/
(Assignee)

Comment 84

4 years ago
I did confirmation the patches on master. But it seems that hw video codec and camera preview does not work, because of regression of Bug 843599.
(Assignee)

Comment 85

4 years ago
Created attachment 758976 [details] [diff] [review]
Part 1a v2 - instantiate MediaResourceManagerService
(Assignee)

Comment 86

4 years ago
Created attachment 758977 [details] [diff] [review]
Part 1b v3 - change to /widget/gonk/Makefile.in
(Assignee)

Comment 87

4 years ago
Created attachment 758978 [details] [diff] [review]
Part 2a v4 - implement MediaResourceManagerService
(Assignee)

Comment 88

4 years ago
Created attachment 758980 [details] [diff] [review]
Part 2b - Makefile.in for mediaresourcemanager
(Assignee)

Comment 89

4 years ago
Created attachment 758981 [details] [diff] [review]
Part 3a - change to HTMLMediaElement
(Assignee)

Comment 90

4 years ago
Created attachment 758982 [details] [diff] [review]
Part 3b v2 - change to content/media/
(Assignee)

Comment 91

4 years ago
Created attachment 758983 [details] [diff] [review]
Part 3c v3 - change to /content/media/omx/
(Assignee)

Comment 92

4 years ago
Created attachment 758984 [details] [diff] [review]
Part 3d - change to Makefile.in for omx
(Assignee)

Comment 93

4 years ago
Created attachment 758985 [details] [diff] [review]
Part 3e - change to layout/build/Makefile.in
(Assignee)

Comment 94

4 years ago
Created attachment 758986 [details] [diff] [review]
Part 3f - change to content/media/moz.build
(Assignee)

Comment 95

4 years ago
Comment on attachment 758976 [details] [diff] [review]
Part 1a v2 - instantiate MediaResourceManagerService

Carry "mwu: review+".
Attachment #758976 - Flags: review+
(Assignee)

Comment 96

4 years ago
Comment on attachment 758977 [details] [diff] [review]
Part 1b v3 - change to /widget/gonk/Makefile.in

Carry "mwu: review+".
Attachment #758977 - Flags: review+
(Assignee)

Comment 97

4 years ago
Comment on attachment 758978 [details] [diff] [review]
Part 2a v4 - implement MediaResourceManagerService

Carry "chris.double: review+".
Attachment #758978 - Flags: review+
(Assignee)

Comment 98

4 years ago
Comment on attachment 758980 [details] [diff] [review]
Part 2b - Makefile.in for mediaresourcemanager

Carry "chris.double: review+".
Attachment #758980 - Flags: review+
(Assignee)

Comment 99

4 years ago
Comment on attachment 758981 [details] [diff] [review]
Part 3a - change to HTMLMediaElement

Carry "chris.double: review+".
Attachment #758981 - Flags: review+
(Assignee)

Comment 100

4 years ago
Comment on attachment 758982 [details] [diff] [review]
Part 3b v2 - change to content/media/

Carry "chris.double: review+".
Attachment #758982 - Flags: review+
(Assignee)

Comment 101

4 years ago
Comment on attachment 758983 [details] [diff] [review]
Part 3c v3 - change to /content/media/omx/

Carry "chris.double: review+".
Attachment #758983 - Flags: review+
(Assignee)

Comment 102

4 years ago
Comment on attachment 758984 [details] [diff] [review]
Part 3d - change to Makefile.in for omx

Carry "chris.double: review+".
Attachment #758984 - Flags: review+
(Assignee)

Comment 103

4 years ago
Comment on attachment 758986 [details] [diff] [review]
Part 3f - change to content/media/moz.build

Carry "chris.double: review+".
Attachment #758986 - Flags: review+
(Assignee)

Comment 104

4 years ago
Comment on attachment 758985 [details] [diff] [review]
Part 3e - change to layout/build/Makefile.in

Cafrry "roc: review+".
Attachment #758985 - Flags: review+
(Assignee)

Comment 105

4 years ago
Created attachment 759197 [details] [diff] [review]
Part 1a v2 for b2g18 - instantiate MediaResourceManagerServic
Attachment #758976 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #758976 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Attachment #758732 - Attachment is obsolete: true
(Assignee)

Comment 106

4 years ago
Created attachment 759200 [details] [diff] [review]
Part 1b v3 for b2g18 - change to /widget/gonk/Makefile.in
Attachment #758743 - Attachment is obsolete: true
Attachment #759200 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759197 - Flags: review+
(Assignee)

Comment 107

4 years ago
Created attachment 759202 [details] [diff] [review]
Part 2a v4 for b2g18 - implement MediaResourceManagerService
Attachment #758750 - Attachment is obsolete: true
(Assignee)

Comment 108

4 years ago
Created attachment 759205 [details] [diff] [review]
Part 2b for b2g18 - Makefile.in for mediaresourcemanager
Attachment #756306 - Attachment is obsolete: true
(Assignee)

Comment 109

4 years ago
Created attachment 759208 [details] [diff] [review]
Part 3a for b2g18 - change to HTMLMediaElement
Attachment #756309 - Attachment is obsolete: true
(Assignee)

Comment 110

4 years ago
Created attachment 759214 [details] [diff] [review]
Part 3b for b2g18 v2 - change to content/media/
Attachment #758789 - Attachment is obsolete: true
(Assignee)

Comment 111

4 years ago
Created attachment 759217 [details] [diff] [review]
Part 3c for b2g18 v3 - change to /content/media/omx/
Attachment #758813 - Attachment is obsolete: true
(Assignee)

Comment 112

4 years ago
Created attachment 759221 [details] [diff] [review]
Part 3d for b2g18 - change to Makefile.in for omx
Attachment #756312 - Attachment is obsolete: true
(Assignee)

Comment 113

4 years ago
Created attachment 759223 [details] [diff] [review]
Part 3e for b2g18 - change to layout/build/Makefile.in
Attachment #756387 - Attachment is obsolete: true
(Assignee)

Comment 114

4 years ago
Created attachment 759224 [details] [diff] [review]
Part 3f for b2g18 - change to content/media/moz.build
Attachment #756901 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759202 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759205 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759208 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759214 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759217 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759221 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759223 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759224 - Flags: review+
(Assignee)

Comment 115

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9ec8013c946e
(Assignee)

Comment 116

4 years ago
Created attachment 759267 [details] [diff] [review]
Part 1a v2 - instantiate MediaResourceManagerService
Attachment #758976 - Attachment is obsolete: true
(Assignee)

Comment 117

4 years ago
Created attachment 759269 [details] [diff] [review]
Part 1b v3 - change to /widget/gonk/Makefile.in
Attachment #758977 - Attachment is obsolete: true
Attachment #759269 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759267 - Flags: review+
(Assignee)

Comment 118

4 years ago
Created attachment 759270 [details] [diff] [review]
Part 2a v4 - implement MediaResourceManagerService
Attachment #758978 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759270 - Flags: review+
(Assignee)

Comment 119

4 years ago
Created attachment 759271 [details] [diff] [review]
Part 2b - Makefile.in for mediaresourcemanager
Attachment #758980 - Attachment is obsolete: true
Attachment #759271 - Flags: review+
(Assignee)

Comment 120

4 years ago
Created attachment 759272 [details] [diff] [review]
Part 3a - change to HTMLMediaElement
Attachment #758981 - Attachment is obsolete: true
Attachment #759272 - Flags: review+
(Assignee)

Comment 121

4 years ago
Created attachment 759273 [details] [diff] [review]
Part 3b v2 - change to content/media/
Attachment #758982 - Attachment is obsolete: true
Attachment #759273 - Flags: review+
(Assignee)

Comment 122

4 years ago
Created attachment 759274 [details] [diff] [review]
Part 3c v3 - change to /content/media/omx/
Attachment #759274 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #758983 - Attachment is obsolete: true
(Assignee)

Comment 123

4 years ago
Created attachment 759276 [details] [diff] [review]
Part 3d - change to Makefile.in for omx
Attachment #758984 - Attachment is obsolete: true
Attachment #759276 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #758985 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #758986 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759197 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759200 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759202 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759205 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759208 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759214 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759217 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759221 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759223 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759224 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759267 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759270 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759269 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759271 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759272 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759273 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759274 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759276 - Attachment is obsolete: true
(Assignee)

Comment 124

4 years ago
Created attachment 759404 [details] [diff] [review]
Part 1a v3 - instantiate MediaResourceManagerService

Fix a nit bug.
(Assignee)

Comment 125

4 years ago
Created attachment 759406 [details] [diff] [review]
Part 1a for b2g18 v3 - instantiate MediaResourceManagerService

Fix a nit bug.
(Assignee)

Comment 126

4 years ago
Created attachment 759409 [details] [diff] [review]
Patch - share hw codec between applications/tasks

Roll up patch.
(Assignee)

Comment 127

4 years ago
Created attachment 759429 [details] [diff] [review]
Patch for b2g18 - share hw codec between applications/tasks

Roll up patch for b2g18.
Attachment #759429 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759409 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #759404 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #759406 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/4c129a5676eb
Keywords: checkin-needed
Backed out for mochitest-1 failures.
https://hg.mozilla.org/projects/birch/rev/cbefbd6f0675

https://tbpl.mozilla.org/php/getParsedLog.php?id=23898314&tree=Birch
(Assignee)

Comment 130

4 years ago
Current patch fails some tests. Needs to fix them.
(Assignee)

Comment 131

4 years ago
Following test was failed.
  > /tests/content/media/test/test_chaining.html

It seems that in ogg and opus, chaining content sends multiple metadataloaded events. The patches restrict to send metadataloaded event only once, because they enable multiple times of codec's load/unload and this could trigger multiple times of metadataloaded events.
(Assignee)

Comment 132

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #131)
> Following test was failed.
>   > /tests/content/media/test/test_chaining.html
> 
> It seems that in ogg and opus, chaining content sends multiple
> metadataloaded events. The patches restrict to send metadataloaded event
> only once, because they enable multiple times of codec's load/unload and
> this could trigger multiple times of metadataloaded events.

By disabling above processing code, test_chaining.html passed on my pc.
(Assignee)

Comment 133

4 years ago
There is no good way to differentiate between chaining content's metadataloaded event and the event triggered by codec's load/unload. I am going to fix this bug by not to suppress "the event triggered by codec's load/unload". If it becomes necessary, create a new bug.
(Assignee)

Comment 134

4 years ago
try tryserver.
https://tbpl.mozilla.org/?tree=Try&rev=84d6043d90f7
(Assignee)

Comment 135

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #134)
> try tryserver.
> https://tbpl.mozilla.org/?tree=Try&rev=84d6043d90f7

Tryserver is down Bug 880740. I asked to rel-eng about ETA, but they do not know about it.
(Assignee)

Comment 136

4 years ago
Created attachment 759842 [details] [diff] [review]
Patch v3 - share hw codec between applications/tasks

Fix test failure.
Attachment #759409 - Attachment is obsolete: true
Attachment #759842 - Flags: review+
(Assignee)

Comment 137

4 years ago
Created attachment 759871 [details] [diff] [review]
Patch v3 for b2g18 - share hw codec between applications/tasks

Fixed test failure.
Attachment #759429 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 880743
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 138

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=88b7c3cd7449
(Assignee)

Comment 139

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #138)
> https://tbpl.mozilla.org/?tree=Try&rev=88b7c3cd7449

almost all green. But There are some oranges like following.
> ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | Test timed out.
(Assignee)

Comment 140

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #139)
> almost all green. But There are some oranges like following.
> > ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_playback.html | Test timed out.

Intermittently test fails. It seems the test failed by "Multichannel Opus in an ogg container". By comment out from test, test pass.
(Assignee)

Comment 141

4 years ago
Created attachment 760169 [details] [diff] [review]
Patch v4 - share hw codec between applications/tasks

Change MediaDecoder's state as much as possible to current code.
Attachment #759842 - Attachment is obsolete: true
(Assignee)

Comment 142

4 years ago
Created attachment 760170 [details] [diff] [review]
Patch v4 for b2g18 - share hw codec between applications/tasks

Change MediaDecoder's state as much as possible to current code.
Attachment #760170 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #760169 - Flags: review+
(Assignee)

Comment 143

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=40fe48ca64fa
(Assignee)

Comment 144

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #143)
> https://tbpl.mozilla.org/?tree=Try&rev=40fe48ca64fa

Intermittent test_playback.html failure disappear.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Component: Gaia::Video → Video/Audio
Product: Boot2Gecko → Core
Attachment #759871 - Attachment is obsolete: true
https://hg.mozilla.org/projects/birch/rev/a7c70ff62b83
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a7c70ff62b83
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
QA Contact: jsmith
https://hg.mozilla.org/releases/mozilla-b2g18/rev/78de618c071a
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
status-firefox24: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/78de618c071a
status-b2g-v1.1hd: affected → fixed

Updated

4 years ago
Keywords: verifyme

Updated

4 years ago
QA Contact: jsmith

Comment 149

4 years ago
This issue does not repro anymore. Refreshing the web page when the video is playing, page refreshes and video starts to play again. But, the video is cut off the screen. 

Verified on 
Leo Build ID: 20130625070217
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/29933d1937db
Gaia: 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Platform Version: 18.1
RIL Version: 01.01.00.019.138
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified

Comment 150

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> Created attachment 751548 [details]
> design diagram of MeiaResourceManager

Sotaro,
 I think MediaResourceManagerService is running in b2g process now,but the diagram show it in Media Server process.How to understand is correct?

Updated

4 years ago
Depends on: 887527
(Assignee)

Comment 151

4 years ago
(In reply to cheng.luo from comment #150)
> (In reply to Sotaro Ikeda [:sotaro] from comment #16)
> > Created attachment 751548 [details]
> > design diagram of MeiaResourceManager
> 
> Sotaro,
>  I think MediaResourceManagerService is running in b2g process now,but the
> diagram show it in Media Server process.How to understand is correct?

The diagram is old. I wrote it before implement the MediaResourceManagerService. "MediaResourceManagerService is running in b2g process" is correct.
(Assignee)

Updated

4 years ago
Attachment #751540 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #751541 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #751548 - Attachment is obsolete: true
(Assignee)

Comment 152

4 years ago
I uploaded the updated diagrams to https://github.com/sotaroikeda/firefox-diagrams/wiki/Firefox-Diagrams

Comment 153

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #151)
> The diagram is old. I wrote it before implement the
> MediaResourceManagerService. "MediaResourceManagerService is running in b2g
> process" is correct.

Thanks.
I found it crashed on my device when first entry video app generating thumbnails.
It request the AudioSource to stop when it have not be started.
In normal, it should be started at OMXCodecProxy::statusChanged on Binder ipc thread.
stack as follow:
#0  __libc_android_abort () at bionic/libc/unistd/abort.c:82
#1  0x400364b8 in __android_log_assert (cond=<value optimized out>, tag=0x4203a897 "MPEG4Extractor", fmt=0x4202e8b1 "%s")
#2  0x41f8146c in android::MPEG4Source::stop (this=0x45406ec0) at frameworks/base/media/libstagefright/MPEG4Extractor.cpp:1932
#3  0x41f92442 in android::OMXCodec::stop (this=0x43fbd900) at frameworks/base/media/libstagefright/OMXCodec.cpp:4242
#4  0x40a22424 in android::OMXCodecProxy::stop (this=<value optimized out>)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/OMXCodecProxy.cpp:207
#5  0x40a20c68 in android::OmxDecoder::ReleaseMediaResources (this=0x423adbc0)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/OmxDecoder.cpp:427
#6  0x40a20850 in mozilla::MediaOmxReader::ReleaseMediaResources (this=0x43350d00)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/MediaOmxReader.cpp:68
#7  0x40891b60 in mozilla::MediaDecoderStateMachine::RunStateMachine (this=0x4237b440)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/MediaDecoderStateMachine.cpp:2152
#8  0x40891eb8 in mozilla::MediaDecoderStateMachine::CallRunStateMachine (this=0x4237b440)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/MediaDecoderStateMachine.cpp:2729
#9  0x40891f38 in mozilla::MediaDecoderStateMachine::Run (this=0x4237b440)
    at /home/apuser/Mozilla/version_update/B2G/gecko/content/media/MediaDecoderStateMachine.cpp:2706
#10 0x40ed874a in nsThread::ProcessNextEvent (this=0x44272ac0, mayWait=<value optimized out>, result=0x44b3eeaf)
(Assignee)

Comment 154

4 years ago
(In reply to cheng.luo from comment #153)
> (In reply to Sotaro Ikeda [:sotaro] from comment #151)
> > The diagram is old. I wrote it before implement the
> > MediaResourceManagerService. "MediaResourceManagerService is running in b2g
> > process" is correct.
> 
> Thanks.
> I found it crashed on my device when first entry video app generating
> thumbnails.
> It request the AudioSource to stop when it have not be started.
> In normal, it should be started at OMXCodecProxy::statusChanged on Binder
> ipc thread.
> stack as follow:
> #0  __libc_android_abort () at bionic/libc/unistd/abort.c:82
> #1  0x400364b8 in __android_log_assert (cond=<value optimized out>,
> tag=0x4203a897 "MPEG4Extractor", fmt=0x4202e8b1 "%s")
> #2  0x41f8146c in android::MPEG4Source::stop (this=0x45406ec0) at
> frameworks/base/media/libstagefright/MPEG4Extractor.cpp:1932
> #3  0x41f92442 in android::OMXCodec::stop (this=0x43fbd900) at
> frameworks/base/media/libstagefright/OMXCodec.cpp:4242
> #4  0x40a22424 in android::OMXCodecProxy::stop (this=<value optimized out>)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/
> OMXCodecProxy.cpp:207
> #5  0x40a20c68 in android::OmxDecoder::ReleaseMediaResources
> (this=0x423adbc0)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/OmxDecoder.
> cpp:427
> #6  0x40a20850 in mozilla::MediaOmxReader::ReleaseMediaResources
> (this=0x43350d00)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/omx/
> MediaOmxReader.cpp:68
> #7  0x40891b60 in mozilla::MediaDecoderStateMachine::RunStateMachine
> (this=0x4237b440)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/
> MediaDecoderStateMachine.cpp:2152
> #8  0x40891eb8 in mozilla::MediaDecoderStateMachine::CallRunStateMachine
> (this=0x4237b440)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/
> MediaDecoderStateMachine.cpp:2729
> #9  0x40891f38 in mozilla::MediaDecoderStateMachine::Run (this=0x4237b440)
>     at
> /home/apuser/Mozilla/version_update/B2G/gecko/content/media/
> MediaDecoderStateMachine.cpp:2706
> #10 0x40ed874a in nsThread::ProcessNextEvent (this=0x44272ac0,
> mayWait=<value optimized out>, result=0x44b3eeaf)

Isn't it dup of Bug 887527?

Comment 155

4 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #154)
Isn't it dup of Bug 887527?

I think so,Thanks
Duplicate of this bug: 888724
Duplicate of this bug: 888723
Duplicate of this bug: 888716

Updated

4 years ago
Flags: in-moztrap?
(Assignee)

Updated

3 years ago
See Also: → bug 1107348
You need to log in before you can comment on or make changes to this bug.