Cannot run multiple HW encoder+decoder at the same time so we should support falling back to use SW codec if HW is already in use.

Hi Maire, Do you think whether or not we need to handle this case for the webRTC in v2.0?

Yes we do.

I need to make a similar change for OpenH264 support (enable or disable VP8 support when generating offers or answers depending on availability). We can enable/disable H264 SDP support depending on whether the encoder/decoder are in use at CreateOffer/Answer time.

Based on comments 2 & 3, mark this bug with feature-b2g and block H.264 user story meta bug.

MediaResourceManagerService is how media playback manages HW codec availability. I'd like to extend it, for WebRTC, to support H.264 encoder and client that won't wait. Please let me know what do you think about this idea, thanks.

unbitrot

Comment on attachment 8429744 [details] [diff] [review] [WIP] Proposal to extend MediaResourceManagerService for querying availability of HW codec. > // Request a media resource for IMediaResourceManagerClient. > virtual void requestMediaResource(const sp<IMediaResourceManagerClient>& client, int resourceType) = 0; >+ // Request a media resource for IMediaResourceManagerClient without waiting. >+ // If the resouce is in use, return ERROR_RESOURCE_NOT_AVAILABLE, otherwise >+ // return OK. When returning OK, client's statusChanged() callback will be >+ // called with MediaResourceManagerClient::CLIENT_STATE_RESOURCE_ASSIGNED. >+ virtual status_t requestMediaResourceNoWait(const sp<IMediaResourceManagerClient>& client, int resourceType) = 0; Another way is to add one more parameter in the original API, like requestMediaResource(const sp<IMediaResourceManagerClient>& client, int resourceType, bool wait) = 0;But this requires to modify the existing used codes. >--- a/content/media/omx/mediaresourcemanager/MediaResourceManagerService.h >+++ b/content/media/omx/mediaresourcemanager/MediaResourceManagerService.h >@@ -22,32 +22,36 @@ namespace android { > * Manage permissions of using media resources(hw decoder, hw encoder, camera) > * XXX Current implementaion support only one hw video decoder. > * Need to extend to support multiple instance and other resources. > */ > class MediaResourceManagerService: public BnMediaResourceManagerService, > public IBinder::DeathRecipient > { > public: >- // The maximum number of hardware decoders available. >- enum { VIDEO_DECODER_COUNT = 1 }; >+ // The maximum number of hardware resoureces available. >+ enum { >+ VIDEO_DECODER_COUNT = 1, >+ VIDEO_ENCODER_COUNT = 1 >+ }; More and more platforms support more than one instance. Hopefully we can find a better way to know how many instance supported. Bug 978672 is trying to find a better way.

(In reply to Blake Wu [:bwu][:blakewu] from comment #8) > >+ virtual status_t requestMediaResourceNoWait(const sp<IMediaResourceManagerClient>& client, int resourceType) = 0; > Another way is to add one more parameter in the original API, like > requestMediaResource(const sp<IMediaResourceManagerClient>& client, int > resourceType, bool wait) = 0;But this requires to modify the existing used > codes. That might be preferable, but I leave that up to those who know the code where it's used already. > >- // The maximum number of hardware decoders available. > >- enum { VIDEO_DECODER_COUNT = 1 }; > >+ // The maximum number of hardware resoureces available. > >+ enum { > >+ VIDEO_DECODER_COUNT = 1, > >+ VIDEO_ENCODER_COUNT = 1 > >+ }; > More and more platforms support more than one instance. Hopefully we can > find a better way to know how many instance supported. Bug 978672 is trying > to find a better way. That's good to hear (I know the TI OMAP stuff in general always supported multiple encoders/decoders, but I don't know much about all the other chipsets used in Android/B2G). Is there a way to determine it dynamically on first-open? For B2G, given these vendor builds are chipset-specific, it could even be in a system pref. If we're using this for Android, we may need to dynamically test (try opening 2 and see if we get software or hardware, and then record it). To be fancy, on first use we could probe even deeper, and then record the results into a pref to avoid the probe in the future.

Comment on attachment 8429744 [details] [diff] [review] [WIP] Proposal to extend MediaResourceManagerService for querying availability of HW codec. LGTM:-)

Comment on attachment 8431480 [details] [diff] [review] Extend MediaResourceManagerService to support video encoder and non-waiting client. Review of attachment 8431480 [details] [diff] [review]: ----------------------------------------------------------------- Very nice! Thanks ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.h @@ +21,5 @@ > namespace android { > > /** > * Manage permissions of using media resources(hw decoder, hw encoder, camera) > + * XXX Current implementaion support only one hw video codec. "implementation supports" Right. We'll want to support VP8 decode on 8x10 for example - but I suspect it shares resources with H.264 decode. And we'll want to support devices with multiple encoder/decoders, but again that's for later patches.

Implementation disabled until a problem with the dependent patch to MediaResourceManager is fixed (service in parent crashes when I try to release a resource). Temporarily replaced with a local bool array; tested with the SDP changes and works for the primary usecase - making a call while playing a video (simulated by setting the DECODER bool to true)

Also includes a crash fix for when someone sends us an unexpected payload type (most uses of FindExternalDecoder* null-check the result; this didn't)

Found the bug: it was mMap.add()ing the decoder resource twice instead of the decoder and the encoder. Patch will be forthcoming

merged patch

patch revised to get rid of mock impl now the MediaResourceManager works

Comment on attachment 8432653 [details] [diff] [review] extend MediaResourceManagerService to support video encoder and non-waiting client. Re-marking for review. Interdiffs are in the previous patch. Yes, I could use an array... decided not to; less prone to accidental breakage.

Comment on attachment 8432653 [details] [diff] [review] extend MediaResourceManagerService to support video encoder and non-waiting client. Review of attachment 8432653 [details] [diff] [review]: ----------------------------------------------------------------- The patch seems almost good, but I have one question about the patch. ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp @@ +81,2 @@ > sp<IBinder> binder = client->asBinder(); > + mResources.enqueueRequest(binder, type); When willWait is false, from the api definition, it seems that we need to acquire MediaResource until requestMediaResource() function completed. But this patch just check if the resource is available. Why the above code doing it? By the current patch, there could be the cases that other MediaResourceManagerClient get the resource in race condition case.

:jolin, can you answer the question in Comment 20?

Comment on attachment 8432236 [details] [diff] [review] Codec availability support and prioritization Review of attachment 8432236 [details] [diff] [review]: ----------------------------------------------------------------- lgtm - only minor comments. ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +246,5 @@ > } > > +int VcmSIPCCBinding::getVideoCodecsGmp() > +{ > + // if (!mGmpCodecsLoaded) { LoadGmpCodecs(); } Reminder to fix or remove this. @@ +255,5 @@ > +{ > + // Check to see if what HW codecs are available (not in use) at this moment. > + // Note that streaming video decode can reserve a decoder > + > + // XXX See bug XXXXXXX Implement W3 codec reservation policy Reminder to put in the bug number here @@ +264,5 @@ > + // the call actually starts, use the reservation but ensure it's held during reconfigs > + // like when the resolution changes, which implies it should not be held in the actual > + // encoder/decoder, but in call control, or PCImpl/Media/etc. > + // Note that currently, OMXCodecReservation needs to be held by an sp<> because it puts > + // 'this' into an sp<EventListener> to talk to the resource reservation code The real solution to this is complex and this comment didn't make it particularly clear to me. Perhaps instead we should point to the bug and put a comment here about what the behavior will be with this patch. I'm guessing this patch will reserve the hardware codec at the first call to createOffer/Answer and will hold it until video teardown. Also if you call createOffer twice in a row I'm guessing the second would not have the hw codec. Perhaps I have those details wrong, but it would be better to put the expected behavior of this patch here.

(In reply to Sotaro Ikeda [:sotaro] from comment #20) > Comment on attachment 8432653 [details] [diff] [review] > extend MediaResourceManagerService to support video encoder and non-waiting > client. > > Review of attachment 8432653 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch seems almost good, but I have one question about the patch. > > ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp > @@ +81,2 @@ > > sp<IBinder> binder = client->asBinder(); > > + mResources.enqueueRequest(binder, type); > > When willWait is false, from the api definition, it seems that we need to > acquire MediaResource until requestMediaResource() function completed. But > this patch just check if the resource is available. Why the above code doing > it? By the current patch, there could be the cases that other > MediaResourceManagerClient get the resource in race condition case. You're right. The lock should be hold until end of function. Will correct it in next vesion. Thanks!

Comment on attachment 8432656 [details] [diff] [review] Use Media Resource Manager to reserve OMX Codecs Review of attachment 8432656 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: content/media/omx/OMXCodecWrapper.cpp @@ +36,5 @@ > > +bool > +OMXCodecReservation::ReserveOMXCodec() > +{ > + CODEC_ERROR("requesting codec type %d", (int) mType); Looks like debug log. Do you want to land it as error message? @@ +53,5 @@ > + > +void > +OMXCodecReservation::ReleaseOMXCodec() > +{ > + CODEC_ERROR("releasing codec type %d", (int) mType); Same as above.

- merge jesup's fixes - address Sotaro's review comments - fix an issue that cancelClient() fails to transfer resource ownership to next in line. - fix build break (due to KeyVector::operator[] and Vector::resize() are missing in ICS)

upload the correct patch.

Comment on attachment 8432656 [details] [diff] [review] Use Media Resource Manager to reserve OMX Codecs Undo mis-obsoletion.

Try result of attachment 8433084 [details] [diff] [review]: https://tbpl.mozilla.org/?tree=Try&rev=ba75b660978f

Comment on attachment 8432656 [details] [diff] [review] Use Media Resource Manager to reserve OMX Codecs Review of attachment 8432656 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/OMXCodecWrapper.cpp @@ +44,5 @@ > + > + mManagerService = mClient->getMediaResourceManagerService(); > + if (!mManagerService.get()) { > + mClient = nullptr; > + return true; // not really in use, but not usable Kind of confuse here. Should we return true here?

Comment on attachment 8432656 [details] [diff] [review] Use Media Resource Manager to reserve OMX Codecs Review of attachment 8432656 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp @@ +933,4 @@ > mOMX = nullptr; > + if (hadOMX) { > + mReservation->ReleaseOMXCodec(); > + } Nitty Code in WebrtcOMXH264VideoDecoder::Release() and WebrtcOMXH264VideoEncoder::Release() are not symmetrical. Either call ReleaseOMXCodec directly or do mOMX check before ReleaseOMXCodec in both side.

scenario: Focus for 2.0 is 1:1 calling, future phase multi-person. scenario that needs to be tested is 2.

> - address Sotaro's review comments Can you explain about how you fix the comment? Sorry, I can not find out how to fix the comment. Can you explain about it more? In my understanding, when "bool willWait" is false, requestMediaResource() have to return a request's result. The patch has a following comment about "willWait". > willWait indicates that, when the resource is not currently available (i.e., already in use by another client), if the client wants to wait. My concern is that, if the race condition happens, requestMediaResource() might wait longer time even when "willWait = false". MediaResourceManagerService::requestMediaResource() checks an available resource by calling mResources.findAvailableResource(type) and enqueue the request by calling mResources.enqueueRequest(binder, type), but it does not acquire the resource in the function. The actual resource acquisition is put off to MediaResourceManagerService::onMessageReceived(). Therefore, if the request queue already has another request for the resource, the client seems to wait longer time until the resource acquisition.

(In reply to Sotaro Ikeda [:sotaro] from comment #32) > > - address Sotaro's review comments > > Can you explain about how you fix the comment? Sorry, I can not find out how > to fix the comment. Can you explain about it more? In my understanding, when > "bool willWait" is false, requestMediaResource() have to return a request's > result. The patch has a following comment about "willWait". > > > willWait indicates that, when the resource is not currently available (i.e., already in use by another client), if the client wants to wait. > > My concern is that, if the race condition happens, requestMediaResource() > might wait longer time even when "willWait = false". > > MediaResourceManagerService::requestMediaResource() checks an available > resource by calling mResources.findAvailableResource(type) and enqueue the > request by calling mResources.enqueueRequest(binder, type), but it does not > acquire the resource in the function. The actual resource acquisition is put > off to MediaResourceManagerService::onMessageReceived(). Therefore, if the > request queue already has another request for the resource, the client seems > to wait longer time until the resource acquisition. It does the findAvailableResource() under mLock, and if available enqueues to actually acquire. Since it was locked, no other request can jump ahead of us into the queue. There is one race condition case: an entry is in the queue, but has not been processed yet. It will get the resource when looper runs, and our request won't. (You can't just check for an empty queue, since if there are two of the resource, and one is in the queue, then you may get it anyways.) I'm writing a patch to verify that enough resources to satisfy all outstanding requests are available for the willWait==false case. Done under lock, this ensures that if we enqueue a request we'll get one of the resource.

interdiffs to fix the media resource patch

(In reply to Sotaro Ikeda [:sotaro] from comment #32) > > - address Sotaro's review comments > > Can you explain about how you fix the comment? Sorry, I can not find out how > to fix the comment. Can you explain about it more? In my understanding, when > "bool willWait" is false, requestMediaResource() have to return a request's > result. The patch has a following comment about "willWait". > > > willWait indicates that, when the resource is not currently available (i.e., already in use by another client), if the client wants to wait. > > My concern is that, if the race condition happens, requestMediaResource() > might wait longer time even when "willWait = false". > > MediaResourceManagerService::requestMediaResource() checks an available > resource by calling mResources.findAvailableResource(type) and enqueue the > request by calling mResources.enqueueRequest(binder, type), but it does not > acquire the resource in the function. The actual resource acquisition is put > off to MediaResourceManagerService::onMessageReceived(). Therefore, if the > request queue already has another request for the resource, the client seems > to wait longer time until the resource acquisition. The 'addressed comment' is that |autoLock| didn't protect request enqueuing. A race condition could be: 2 requests, 1st one won't wait and 2nd one will, arrive about the same time. When the non-waiting client pass availability check, the waiting client preempts and is enqueued (therefore will aquire the resource) before the non-waiting client. As Randell say in comment 34, that patch did miss the 'not currently in use but there is someone in line waiting' case. :(

Comment on attachment 8433739 [details] [diff] [review] fix race condition in MediaResourceManager patch Review of attachment 8433739 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Thanks! ::: content/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp @@ +195,5 @@ > return mMap.indexOfKey(type) != NAME_NOT_FOUND; > } > > +ssize_t MediaResourceManagerService::ResourceTable::findAvailableResource(ResourceType type, > + size_t number_needed) s/number_needed/numberNeeded/

Comment on attachment 8433739 [details] [diff] [review] fix race condition in MediaResourceManager patch Looks good! In this patch, how to check the resource availability depends on that the resource is allocated based only on the resource's count. In future, if we want to allocate the resource with other parameter like video size. We need to change the implementation. For example, in some smart phones, actual numbers of hw decoder instances are determined by video size. If the size is small, a lot of instances could be created, but if it is large like full HD, only one instances could be created. In this case, it seems better to allocate resources in MediaResourceManagerService::requestMediaResource() and call a callback function on ALooper thread.

Comment on attachment 8433084 [details] [diff] [review] Extend MediaResourceManagerService to support video encoder and non-waiting client. review+ with attachment 8433739 [details] [diff] [review].

https://hg.mozilla.org/integration/mozilla-inbound/rev/52f23831adbc https://hg.mozilla.org/integration/mozilla-inbound/rev/eb336259b85f https://hg.mozilla.org/integration/mozilla-inbound/rev/9e0dfff3fdb2

https://hg.mozilla.org/mozilla-central/rev/52f23831adbc https://hg.mozilla.org/mozilla-central/rev/eb336259b85f https://hg.mozilla.org/mozilla-central/rev/9e0dfff3fdb2