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

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?

This is a new feature. I am going to discuss about in web rendering work week in Taipei next week(5/20-24).

Bug 871913 is also related to this issue.

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

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

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.

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

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?

Blocks a blocker.

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.

fix nits.

Complete MediaResourceManagerService's basic capabilities implementation. Next, I am going to implement DORMANT state.

Add OMXCodecProxy.cpp and OMXCodecProxy.h.

fix nits bugs.

Add Dormant mode.

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.

- fix bugs in nsBuiltinDecoder's state handling

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

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

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.

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.

disable logout

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

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.

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.

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.

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

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?

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

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.

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

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

remove unrelated part.

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

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

Comment on attachment 758789 [details] [diff] [review] Part 3b v2 for b2g18 - change to content/media/ Carry "chris.double: review+".

Comment on attachment 758732 [details] [diff] [review] Part 1a v2 for b2g18 - instantiate MediaResourceManagerService This looks much simpler in nsAppShell.cpp. Thanks

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

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+".

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

Comment on attachment 758978 [details] [diff] [review] Part 2a v4 - implement MediaResourceManagerService Carry "chris.double: review+".

Comment on attachment 758980 [details] [diff] [review] Part 2b - Makefile.in for mediaresourcemanager Carry "chris.double: review+".

Comment on attachment 758981 [details] [diff] [review] Part 3a - change to HTMLMediaElement Carry "chris.double: review+".

Comment on attachment 758982 [details] [diff] [review] Part 3b v2 - change to content/media/ Carry "chris.double: review+".

Comment on attachment 758983 [details] [diff] [review] Part 3c v3 - change to /content/media/omx/ Carry "chris.double: review+".

Comment on attachment 758984 [details] [diff] [review] Part 3d - change to Makefile.in for omx Carry "chris.double: review+".

Comment on attachment 758986 [details] [diff] [review] Part 3f - change to content/media/moz.build Carry "chris.double: review+".

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