1108477 - [FFOS] Change current video codec resource management

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, defect, P3)

Description

[Blake Wu [:bwu][:blakewu]]

Continue to discuss video codec resource management on workweek in Portland,

Background:
Currently we limit the number of video resource to be one [1]. But most video codecs support more than one instance. We should unleash the power of video codec and make best use of it.
In this way, it should improve 

1. Make app switch faster.
    Ex. Playing a video in video app from playing youtube. 
2. Speed up thumbnail generation
     - By creating one more instance to decode it
     - User can play video immediately during thumbnail generation
3. Multiple videos can be played simultaneously. 
  Ex. WebRTC can run simultaneously with video app
    Performance (fps) may be degraded which depends on platform's capability. 

Solution:
Combined jesup's good idea to notify app when an instance is released. 
I think the procedure to play a video could be changed as below.    
1. APP play a video. 
2. Omx reader tries to create a codec directly. 
3. If it failed to create a codec, continue to wait the new resource management to notify it to try again. This new resource management will notify it when a instance is released. 

So the purpose of this new resource management will be to notify Omx reader when fails to acquire codec to try again when there is an instance released.
At the same time, Gecko will notify APP to wait via a waiting event (bug 1093283)

Comment 1

[Blake Wu [:bwu][:blakewu]]

[1] http://dxr.mozilla.org/mozilla-central/source/dom/media/omx/mediaresourcemanager/MediaResourceManagerService.h#35

Comment 2

[Blake Wu [:bwu][:blakewu]]

Hi Jesup, Sotaro and Cpearce,
May I have your feedback again?

Comment 3

[Sotaro Ikeda [:sotaro]]

The followings seem necessary as requirements.
- Normal web content that uses video tags should works as expected.
- Need to prevent app oom killed situation.
- Need to prevent decode error of supported video.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

If I recall correctly, in Portland we discussed removing the limit of 1 active codec, and detecting when Reader fails to create codec and then firing a "waiting" event to the <video> element.

We can also maintain a list of Readers waiting for a codec, and when a codec is released we re-try to create the codec, and resume the load. This should make existing <video> usage work as expected.

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> If I recall correctly, in Portland we discussed removing the limit of 1
> active codec, and detecting when Reader fails to create codec and then
> firing a "waiting" event to the <video> element.
> 
> We can also maintain a list of Readers waiting for a codec, and when a codec
> is released we re-try to create the codec, and resume the load. This should
> make existing <video> usage work as expected.

Yeah, theoretically, it should work as expected except oom problem, if the device supports multiple codec.

It seems to becomes simpler by enabling multiple codec just depends on android base version, if all recent b2g device supports multiple video codecs.

Comment 6

[Sotaro Ikeda [:sotaro]]

Until ICS, amount of gralloc was limited, therefore out of gralloc problem happened often. Since ion is supported by android since JB, out of gralloc problem becomes very rare.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Until ICS, amount of gralloc was limited, therefore out of gralloc problem
> happened often. Since ion is supported by android since JB, out of gralloc
> problem becomes very rare.

I saw oom problem more than gralloc oom since JB. Therefore we can focus to oom about amount of memory.

Comment 8

[Sotaro Ikeda [:sotaro]]

IIRC, during last weeks talking, someone talked that apple's hw video codec instance has a limitation.

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> IIRC, during last weeks talking, someone talked that apple's hw video codec
> instance has a limitation.

That was me... The Apple HW decoder will only allow a set amount of instances depending on the HW in use and the OS. This was a problem at some stage as it was the only decoder we had (on 10.6). However, in the event we can't create a HW decoder, we will default to opening a SW decoder. So this limitation isn't that important anymore.

Comment 10

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> The followings seem necessary as requirements.
> - Normal web content that uses video tags should works as expected.
> - Need to prevent app oom killed situation.
We should let APP killed if OOM and may not need to have special handle for video case.
> - Need to prevent decode error of supported video.
If we fail to create codec, that should be the reason that all the instances are used or insufficient memory. This problem may disappear if we re-try after some instance is released. 
If we create codec successfully but get a error when decoding encoded data, that should be a decode error which even we re-try, the problem will not go away.

Comment 11

[Blake Wu [:bwu][:blakewu]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> If I recall correctly, in Portland we discussed removing the limit of 1
> active codec, and detecting when Reader fails to create codec and then
> firing a "waiting" event to the <video> element.
Yes. So there is no a fixed number for the number of instances and an "initial check" to estimate the number instance (mentioned by jesup) would not be required.  
> 
> We can also maintain a list of Readers waiting for a codec, and when a codec
> is released we re-try to create the codec, and resume the load. This should
> make existing <video> usage work as expected.
IMHO, we can try to allocate the codec first. If fails, we can use the current waiting mechanism to wait the resource. It should not change current codes a lot.

Comment 12

[Blake Wu [:bwu][:blakewu]]

Attachment: Video Codec Resource Management.pdf

New Video Codec Resource Management (VCRM) design diagram and the comparison.

Comment 13

[Blake Wu [:bwu][:blakewu]]

So the new design will not change the codes in XXXDecoderReader, MediaDecoder, and those above them. 
Changes will be required in XXXCodecProxy and Current VCRM codes.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

This looks good.  You should document in the design how OOM is handled per the comments above.

Comment 15

[Blake Wu [:bwu][:blakewu]]

(In reply to Randell Jesup [:jesup] from comment #14)
> This looks good.  You should document in the design how OOM is handled per
> the comments above.
I think we should not need to specially handle OOM for video case. If OOM happens, user can open the app again, just like other apps which need to require much memory and could be killed due to OOM. 
Is there any cases that we cannot let it be killed?

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Blake Wu [:bwu][:blakewu] from comment #15)
> I think we should not need to specially handle OOM for video case. If OOM
> happens, user can open the app again, just like other apps which need to
> require much memory and could be killed due to OOM. 
> Is there any cases that we cannot let it be killed?

I'm no expert on how OOMs are handled in B2G, but this seems reasonable.  I was just suggesting adding a clear discussion/example of how OOM would impact this.

Comment 17

[Blake Wu [:bwu][:blakewu]]

(In reply to Randell Jesup [:jesup] from comment #16)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #15)
> > I think we should not need to specially handle OOM for video case. If OOM
> > happens, user can open the app again, just like other apps which need to
> > require much memory and could be killed due to OOM. 
> > Is there any cases that we cannot let it be killed?
> 
> I'm no expert on how OOMs are handled in B2G, but this seems reasonable.  I
> was just suggesting adding a clear discussion/example of how OOM would
> impact this.
Thanks for your suggestion. I will add a discussion about how OOM would impact this and what users may face.

Comment 18

[Blake Wu [:bwu][:blakewu]]

Attachment: Video Codec Resource Management v2.pdf

Change interface names.

Comment 19

[Blake Wu [:bwu][:blakewu]]

Attachment: bug-1108477-WIP.patch

1. Change the current design according to attachment 8542900 [details]. 
2. This draft patch is only for OMXCodecProxy. Once there is no other concerns, I will provide a formal patch for review.

Comment 20

[Blake Wu [:bwu][:blakewu]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #19)
> Created attachment 8542901 [details] [diff] [review]
> bug-1108477-WIP.patch
> 
> 1. Change the current design according to attachment 8542900 [details]. 
> 2. This draft patch is only for OMXCodecProxy. Once there is no other
> concerns, I will provide a formal patch for review.
Add one more. 
3. This patch doesn't consider the case that decoder and encoder share the same codec resource.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Randell Jesup [:jesup] from comment #16)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #15)
> > I think we should not need to specially handle OOM for video case. If OOM
> > happens, user can open the app again, just like other apps which need to
> > require much memory and could be killed due to OOM. 
> > Is there any cases that we cannot let it be killed?
> 
> I'm no expert on how OOMs are handled in B2G, but this seems reasonable.  I
> was just suggesting adding a clear discussion/example of how OOM would
> impact this.

I do not think it is acceptable in general in b2g. On b2g, when low memory is detected by LMK, LMK kills application depends on priority. When memory consumption is huge, foreground application is also killed. If it always happens on a web content. The b2g device can not open the web content.

Comment 22

[Sotaro Ikeda [:sotaro]]

Related to comment 21, it might better to check what happens on the following situation.
- Flame-kk 319MB RAM.
- Load a web content that have a lot of 720p videos
  When the content is loaded, all videos are tried to playback

Comment 23

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8542901 [details] [diff] [review]
bug-1108477-WIP.patch

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

The patch looks very different than I expected. The patch seems not handle the following correctly.
- As we discussed in Portland, ICS should limit the decoder instance to one as currently.
    + b2g ics devices can not handle multiple decoder instances.
- The code should have a capability to support both with decoder instance limitation and without the limitation.
- Previous decoder requests should have a precedence than new one.

::: dom/media/omx/OMXCodecProxy.cpp
@@ +127,4 @@
>      return;
>    }
>  
> +  mOMXCodec = OMXCodec::Create(mOMX, mSrcMeta, mIsEncoder, mSource, mComponentName, mFlags, mNativeWindow);

I prefer that older requests have a precedence of decoder allocation than a new request. This code seems to allow precedence to new decoder allocation request.

@@ +132,5 @@
> +  if (mOMXCodec != nullptr) {
> +    if (mOMXCodec->start() != OK) {
> +      NS_WARNING("Couldn't start OMX video source");
> +      mOMXCodec.clear();
> +      mState = MediaResourceManagerClient::CLIENT_STATE_SHUTDOWN;

OMXCodec's port buffer allocation happens under OMXCodec::start(). Therefore this code does not handle correctly out of gralloc situation.

OMXCodec::start()
 ->OMXCodec::init()
  ->OMXCodec::allocateBuffers()

::: dom/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp
@@ +84,5 @@
> +
> +status_t MediaResourceManagerService::registerClient(const sp<IMediaResourceManagerClient>& client,
> +                                                   int resourceType)
> +{
> +  ALOGD("registerClient");

Some ICS source(default AOSP) does not support ALOG.

Comment 24

[Blake Wu [:bwu][:blakewu]]

Thanks for your feedback!
(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> Comment on attachment 8542901 [details] [diff] [review]
> bug-1108477-WIP.patch
> 
> Review of attachment 8542901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks very different than I expected. The patch seems not handle
> the following correctly.
> - As we discussed in Portland, ICS should limit the decoder instance to one
> as currently.
AFAIK, the limitation should depend on partner's codec, not Android's version. 
Take Nexus 4 for example, even with ICS it can support 4 instances. 
>     + b2g ics devices can not handle multiple decoder instances.
> - The code should have a capability to support both with decoder instance
> limitation and without the limitation.
Agree. I am thinking to have a pref to enable/disable this limitation of instances creation. 
A better way is to know the size of system memory and decide to enable it or not. I will start with a pref first.  
> - Previous decoder requests should have a precedence than new one.
> 
> ::: dom/media/omx/OMXCodecProxy.cpp
> @@ +127,4 @@
> >      return;
> >    }
> >  
> > +  mOMXCodec = OMXCodec::Create(mOMX, mSrcMeta, mIsEncoder, mSource, mComponentName, mFlags, mNativeWindow);
> 
> I prefer that older requests have a precedence of decoder allocation than a
> new request. This code seems to allow precedence to new decoder allocation
> request.
My codes are intended to allow the older requests to have the highest priority to create codec. 
But I noticed there is some chance that when codec resource is full and someone releases his instance, if at this time some app wants to play video and tries to codec, it may take it and the waiting app may fail to get it. Did you mean this?   
> 
> @@ +132,5 @@
> > +  if (mOMXCodec != nullptr) {
> > +    if (mOMXCodec->start() != OK) {
> > +      NS_WARNING("Couldn't start OMX video source");
> > +      mOMXCodec.clear();
> > +      mState = MediaResourceManagerClient::CLIENT_STATE_SHUTDOWN;
> 
> OMXCodec's port buffer allocation happens under OMXCodec::start(). Therefore
> this code does not handle correctly out of gralloc situation.
I should not change the order of calling OMXCodec's function as you mentioned. What did you mean by out of gralloc situation?
> 
> OMXCodec::start()
>  ->OMXCodec::init()
>   ->OMXCodec::allocateBuffers()
> 
> ::: dom/media/omx/mediaresourcemanager/MediaResourceManagerService.cpp
> @@ +84,5 @@
> > +
> > +status_t MediaResourceManagerService::registerClient(const sp<IMediaResourceManagerClient>& client,
> > +                                                   int resourceType)
> > +{
> > +  ALOGD("registerClient");
> 
> Some ICS source(default AOSP) does not support ALOG.
Thanks! I will check it.

Comment 25

[Blake Wu [:bwu][:blakewu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> Related to comment 21, it might better to check what happens on the
> following situation.
> - Flame-kk 319MB RAM.
As comment 24, we may disable/enable the limitation mechanism according to system memory. 
> - Load a web content that have a lot of 720p videos
>   When the content is loaded, all videos are tried to playback
If there is only 4 codec instance supported, only 4 videos will be played. Others will wait for one of them to release codec and then use it.

Comment 26

[Blake Wu [:bwu][:blakewu]]

Attachment: Video Codec Resource Management v3.pdf

I modified the design as attachment. 
It should fix the problem addressed in comment 23 that is "older requests have a precedence of decoder allocation than a new request".

Comment 27

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8562489 [details]
Video Codec Resource Management v3.pdf

XXXCodecProxy seems at fist try to allocate Video codec, even when there is a waiting XXXCodecProxy that are previously failed to allocate the video codec. I think that the previous failed one should have a privilege than new one.

This diagram does not say about how to prevent the risk of a content process is killed by low memory killer because of using too much memory on low memory device.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Where do we stand with this?

Comment 29

[Blake Wu [:bwu][:blakewu]]

(In reply to Randell Jesup [:jesup] from comment #28)
> Where do we stand with this?
Sorry. I don't understand what you mean. Could you elaborate it?

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Blake Wu [:bwu][:blakewu] from comment #29)
> (In reply to Randell Jesup [:jesup] from comment #28)
> > Where do we stand with this?
> Sorry. I don't understand what you mean. Could you elaborate it?

There was work towards a patch we can commit, but this bug seems to have stalled last spring.  Sotaro committed a patch this summer that covers at least some of it, so I was wondering what is still needed, and what state any patches are.