Closed Bug 871485 Opened 11 years ago Closed 11 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
1.1 QE2 (6jun)
Project Flags:
blocking-b2g leo+
Tracking Flags:
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

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

References

Details

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

Attachments

(2 files, 64 obsolete files)

Patch v4 - share hw codec between applications/tasks
80.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Patch v4 for b2g18 - share hw codec between applications/tasks
83.33 KB, patch
sotaro
: review+
Details | Diff | Splinter Review 
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
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
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)
Target Milestone: --- → 1.1 QE2
Bug 871913 is also related to this issue.
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)
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)
Whiteboard: c=video
Status: NEW → ASSIGNED
(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)
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.
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.
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?
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.
The current state diagrams of nsBuiltinDecoderStateMachine and nsBuiltinDecoder on v1.01 is at https://github.com/sotaroikeda/firefox-diagrams/wiki/Firefox-Diagrams
Sotaro, will you fall back on the software decoder when the hardware decoder is in use?
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.
In audio case, it is already fallbacked to sw codec.
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+
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)
fix nits.
Attachment #753263 - Attachment is obsolete: true
Complete MediaResourceManagerService's basic capabilities implementation. Next, I am going to implement DORMANT state.
Add OMXCodecProxy.cpp and OMXCodecProxy.h.
Attachment #753266 - Attachment is obsolete: true
fix nits bugs.
Attachment #753329 - Attachment is obsolete: true
Add Dormant mode.
Attachment #753611 - Attachment is obsolete: true
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.
Whiteboard: c=video → c=video , MiniWW
- fix bugs in nsBuiltinDecoder's state handling
Attachment #754665 - Attachment is obsolete: true
Current patches works well in normal use cases. But one problem is recognized.
 - often see green video frame during changing active videos
(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.
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().
Depends on: 877400
(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.
Whiteboard: c=video , MiniWW → c=video , MiniWW, [TD-26566]
current MediaResourceManagerService do not need to be in mediaserver porocess. To make code management easier, try to move the MediaResourceManagerService in b2g process.
(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.
Attachment #753261 - Attachment is obsolete: true
Attachment #753262 - Attachment is obsolete: true
Attachment #755115 - Attachment is obsolete: true
Attachment #756302 - Flags: review?(mwu)
Attachment #756303 - Flags: review?(mwu)
Attachment #756304 - Flags: review?(chris.double)
Attachment #756306 - Flags: review?(chris.double)
Attachment #756309 - Flags: review?(chris.double)
Attachment #756310 - Flags: review?(chris.double)
Attachment #756311 - Flags: review?(chris.double)
Attachment #756312 - Flags: review?(chris.double)
Attachment #756387 - Flags: review?(chris.double)
Attachment #756901 - Flags: review?(chris.double)
After adapting this patch, I cannot see problem...
But It seems like that youtube got another problem.

it's lagged so much..
It occurred more frequently after patching Bug 870564
(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.
(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??
(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.
(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
disable logout.
Attachment #756304 - Attachment is obsolete: true
Attachment #756304 - Flags: review?(chris.double)
disable logout
Attachment #756311 - Attachment is obsolete: true
Attachment #756311 - Flags: review?(chris.double)
Attachment #757163 - Flags: review?(chris.double)
Attachment #757164 - Flags: review?(chris.double)
(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?
(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.
Attachment #756306 - Flags: review?(chris.double) → review+
Attachment #756309 - Flags: review?(chris.double) → review+
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 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+
Attachment #756312 - Flags: review?(chris.double) → review+
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)
Attachment #756901 - Flags: review?(chris.double) → review+
Attachment #756303 - Flags: review?(mwu) → review+
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 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?
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)
(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.
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();
}
Apply comments.
Attachment #757163 - Attachment is obsolete: true
(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?
Flags: needinfo?(mwu)
Flags: needinfo?(mwu)
(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().
Change the place of resource manager instantiation from from manin() to nsAppShell::Init().
Attachment #756302 - Attachment is obsolete: true
Attachment #756302 - Flags: review?(mwu)

        Attachment #756303 -
        Attachment is obsolete: true

    
    

    
  


  
remove unrelated part.

        Attachment #758735 -
        Attachment is obsolete: true

        Attachment #758732 -
        Flags: review?(mwu)

        Attachment #758743 -
        Flags: review?(mwu)

    
    

    
  


  
Update MediaResourceManagerService::instantiate().
Carry "chris.double: review+"

        Attachment #758750 -
        Flags: review+

        Attachment #758692 -
        Attachment is obsolete: true

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  


  
Apply comment.

        Attachment #756310 -
        Attachment is obsolete: true

    
    

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

Carry "chris.double: review+".

        Attachment #758789 -
        Flags: review+

    
  

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

    
    

    
  
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+

    
    

    
  
(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.

    
    

    
  


  
Apply comment. Carry "chris.double: review+".

        Attachment #757164 -
        Attachment is obsolete: true

        Attachment #756306 -
        Attachment description: Part 2b - Makefile.in for mediaresourcemanager → Part 2b for b2g18 - Makefile.in for mediaresourcemanager

        Attachment #756309 -
        Attachment description: Part 3a - change to nsHTMLMediaElement → Part 3a for b2g18 - change to nsHTMLMediaElement

        Attachment #756312 -
        Attachment description: Part 3d - change to Makefile.in for omx → Part 3d for b2g18 - change to Makefile.in for omx

        Attachment #756387 -
        Attachment description: Part 3e - change to layout/build/Makefile.in → Part 3e for b2g18 - change to layout/build/Makefile.in

        Attachment #756901 -
        Attachment description: Part 3f - change to content/media/Makefile.in → Part 3f for b2g18 - change to content/media/Makefile.in

        Attachment #758732 -
        Attachment description: Part 1a v2 - instantiate MediaResourceManagerService → Part 1a for b2g18 v2 - instantiate MediaResourceManagerService

        Attachment #758743 -
        Attachment description: Part 1b v3 - change to /widget/gonk/Makefile.in → Part 1b v3 for b2g18 - change to /widget/gonk/Makefile.in

        Attachment #758732 -
        Attachment description: Part 1a for b2g18 v2 - instantiate MediaResourceManagerService → Part 1a v2 for b2g18 - instantiate MediaResourceManagerService

        Attachment #758750 -
        Attachment description: Part 2a v4 - implement MediaResourceManagerService → Part 2a v4 for b2g18 - implement MediaResourceManagerService

        Attachment #758789 -
        Attachment description: Part 3b v2 - change to content/media/ → Part 3b v2 for b2g18 - change to content/media/

        Attachment #758813 -
        Attachment description: Part 3c v3 - change to /content/media/omx/ → Part 3c v3 for b2g18 - change to /content/media/omx/

    
    

    
  
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.

    
    

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

Carry "mwu: review+".

        Attachment #758976 -
        Flags: review+

    
    

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

Carry "mwu: review+".

        Attachment #758977 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758978 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758980 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758981 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758982 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758983 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758984 -
        Flags: review+

    
    

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

Carry "chris.double: review+".

        Attachment #758986 -
        Flags: review+

    
    

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

Cafrry "roc: review+".

        Attachment #758985 -
        Flags: review+

        Attachment #758976 -
        Attachment is obsolete: false

        Attachment #758732 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #756309 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #758977 -
        Attachment is obsolete: true

        Attachment #759269 -
        Flags: review+

    
    

    
  


  

        Attachment #758980 -
        Attachment is obsolete: true

        Attachment #759271 -
        Flags: review+

    
    

    
  


  

        Attachment #758981 -
        Attachment is obsolete: true

        Attachment #759272 -
        Flags: review+

    
    

    
  


  

        Attachment #758982 -
        Attachment is obsolete: true

        Attachment #759273 -
        Flags: review+

        Attachment #758983 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #758984 -
        Attachment is obsolete: true

        Attachment #759276 -
        Flags: review+

        Attachment #758985 -
        Attachment is obsolete: true

        Attachment #758986 -
        Attachment is obsolete: true

        Attachment #759197 -
        Attachment is obsolete: true

        Attachment #759200 -
        Attachment is obsolete: true

        Attachment #759202 -
        Attachment is obsolete: true

        Attachment #759205 -
        Attachment is obsolete: true

        Attachment #759208 -
        Attachment is obsolete: true

        Attachment #759214 -
        Attachment is obsolete: true

        Attachment #759217 -
        Attachment is obsolete: true

        Attachment #759221 -
        Attachment is obsolete: true

        Attachment #759223 -
        Attachment is obsolete: true

        Attachment #759224 -
        Attachment is obsolete: true

        Attachment #759267 -
        Attachment is obsolete: true

        Attachment #759270 -
        Attachment is obsolete: true

        Attachment #759269 -
        Attachment is obsolete: true

        Attachment #759271 -
        Attachment is obsolete: true

        Attachment #759272 -
        Attachment is obsolete: true

        Attachment #759273 -
        Attachment is obsolete: true

        Attachment #759274 -
        Attachment is obsolete: true

        Attachment #759276 -
        Attachment is obsolete: true

    
    

    
  


  
Roll up patch for b2g18.

        Attachment #759429 -
        Flags: review+

        Attachment #759404 -
        Attachment is obsolete: true

        Attachment #759406 -
        Attachment is obsolete: true

    
    

    
  
Current patch fails some tests. Needs to fix them.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  


  
Fix test failure.

        Attachment #759409 -
        Attachment is obsolete: true

        Attachment #759842 -
        Flags: review+

    
    

    
  


  
Fixed test failure.

        Attachment #759429 -
        Attachment is obsolete: true
Blocks: 880743

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  


  
Change MediaDecoder's state as much as possible to current code.

        Attachment #759842 -
        Attachment is obsolete: true

    
    

    
  


  
Change MediaDecoder's state as much as possible to current code.

        Attachment #760170 -
        Flags: review+

    
    

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

Intermittent test_playback.html failure disappear.
Component: Gaia::Video → Video/Audio
Product: Boot2Gecko → Core

        Attachment #759871 -
        Attachment is obsolete: true

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a7c70ff62b83
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Keywords: verifyme
QA Contact: jsmith

    
    

    
  
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

    
    

    
  
(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?

    
  
Depends on: 887527

    
    

    
  
(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.

        Attachment #751540 -
        Attachment is obsolete: true

        Attachment #751541 -
        Attachment is obsolete: true

        Attachment #751548 -
        Attachment is obsolete: true

    
    

    
  
(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)

    
    

    
  
(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?

    
    

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

I think so,Thanks
Flags: in-moztrap?
See Also:  → 1107348

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: