Sotaro Ikeda [:sotaro] Reporter
	Description • 11 years ago

Current B2G uses OMXCodec for H.264 video decoding. It's API is synchronous. This makes a problem like Bug 877461 comment #54 and Bug 898866 in streaming video playback cases. In android, ACodec is implemented to fix such problems.

It seems better to use ACodec instead of OMXCodec for video decoding.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 1 • 11 years ago

I am going to unassign myself. I am going to come back if I have a time. But anyone can take this bug.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 2 • 11 years ago

Codec could be MediaCodec or ACodec. MediaCodec is a wrapper of ACodec. MediaCodec seems easier to use.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 3 • 11 years ago

I do not investigated about using MediaCodec/ACodec. But I recognize some problems using OMXCodec.

- OMXCodec reads encoded data from a data source by using synchronous API by holding mutex.

- If http read takes very long time, OMXCodec::read() could failed because of timed out.
   It happens depend on the timing of calling OMXCodec::read().
   By the timeout, on some platforms, OMXCodec's state falls into Error state and
   does not do decoding anymore.
   It is actually almost fixed by Bug 877024. But the cause of the problem is still present.
   If network connection is not good, the problem could still happens.

- Even when the timeout does not cause "the OMXCodec do not work anymore",
   it is better to avoid timeout, timeout cause decodeing thread pending several seconds.

- If http data read takes longer time, because of network condition,
    OMXCodec continue to hold the mutex very long time.

- During the data read, the holding mutex made OMXCodec blocking almost OMXCodec's task.
  "Used video frames" can not be returned to OMXCodec.
  If OMXCodec::read() is blocked and decoding thread is also blocked.
  For example, this could block decoding thread shutdown.
  Decode thread shutdown is synchronously controlled by MediaDecoderStateMachine.
  MediaDecoderStateMachine thread is blocked with holding MediaDecoder's ReentrantMonitor.
  The holding ReentrantMonitor blocked main thread's access to MediaDecoder. 

- b2g's video seeks is inefficient.
   See Bug 919590 Comment 36 and Bug 919590 Comment 37.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 4 • 11 years ago

Supplement to Comment 3
- If OMXCodec free is delayed, It affect to another app's OMXCodec allocation.

	Bruce Sun [:brsun] Assignee
	Comment 5 • 11 years ago

Investigate MediaCodec and ACodec first.

	Bruce Sun [:brsun] Assignee
	Comment 6 • 11 years ago

Investigation on ACodec:
 - Like OMXCodec, ACodec is used to wrap OMX IL components of AOSP.
 - Each public API of ACodec is asynchronous. Clients of ACodec need to register one ALooper with this ACodec to ALooperRoster in advance to process all asynchronous tasks. AMessage is a dictionary-like data container which can carry key-value pairs between clients and ACodec.

Investigation on MediaCodec:
 - MediaCodec is a wrapper class of ACodec on AOSP JB and after.
 - Each public API of MediaCodec is synchronous. MediaCodec prepares the required ALooper(s) for itself and ACodec. Clients of MediaCodec have abilities to set timeout values for buffer-management APIs, and MediaCodec will wait asynchronous replies of AMessage with the specified timeout period at most.

If we want to leverage the benefit of ACodec, to integrate MediaCodec would be a better choice than to integrate ACodec directly:
 - Most common works to handle asynchronous tasks contains in MediaCodec already.
 - Synchronous APIs with timeout capabilities are what we want to use in gecko.
 - APIs of MediaCodec are simpler and easier to be integrated than APIs of ACodec.

	Bruce Sun [:brsun] Assignee
	Comment 9 • 11 years ago

In current design, the consumer thread of OMXCodec, which is the decoding thread, would be blocked by the producer thread of OMXCodec easily if android::MediaSource::read() keeps waiting media data from the network (ex. through HTTP or RTSP) under bad network conditions. There are two kinds of scenarios that we might encounter:
 - The producer of OMXCodec is the decoding thread itself. If OMXCodec::read() is called at the first time on this OMXCodec instance, the producer part will use android::MediaSource::read() to get enough encoded data for decoding. Since the producer part runs on the decoding thread in this case, the consumer part will have no chance to be executed if the producer part is blocked on android::MediaSource::read() function.
 - The producer of OMXCodec is the binder thread. If OMXCodecObserver::onMessage() is called on the binder thread to handle omx_message::EMPTY_BUFFER_DONE, OMXCodec::mLock will be locked first and then the producer part will call android::MediaSource::read(). Since OMXCodec::mLock should be acquired at the very beginning of OMXCodec::read() before the consumer part can be executed, the decoding thread would be blocked on waiting OMXCodec::mLock if the producer part is blocked on android::MediaSource::read().

If the decoding thread is blocked, seeking operations cannot be handled and the decoding thread also cannot be shutdown gracefully at this moment. In order to overcome such problem, we have to isolate the consumer thread from the producer thread so that the consumer thread won't be blocked even if the producer thread keeps waiting for something.

By using MediaCodec, we can isolate the consumer thread from the producer thread as we want. The decoding thread can specify one timeout value for MediaCodec::dequeueOutputBuffer() to avoid such blocking issue that has just been described above. Since the producer part cannot exist on the decoding thread (for isolation, apparently) nor on the binder thread (by the design of MediaCodec) if we want to solve this problem, we have to introduce one thread to drive the producer part.

There are two different proposals in attachment 8351339 [details] to illustrate how we can integrate MediaCodec with one additional producer thread:
 - #1: Simply create one thread to handle the original producer part to pull encoded data from MediaSource to MediaCodec.
   = Pros: Original MediaResource and MediaSource can still be used in this new model as usual. All of these classes are designed to be used under pull mode, so there might be less efforts to integrate it.
   = Cons: When the newly added producer thread is blocked by android::MediaSource::read() due to bad network conditions, we still cannot shutdown this new thread gracefully at this moment. Unfortunately, the potential shutdown problem still exists in this new model as long as android::MediaSource::read() is used.
 - #2: Create a special MediaResource that has one thread inside to push data downstream to a special container parser and then downstream to MediaCodec.
   = Pros: Since the pulling function, android::MediaSource::read(), will not be used in this push model, the blocking condition won't exist anymore. The producer part is executed only if there are additional data from the network. Moreover, if there is one thread already in the existing MediaResource to retrieve data from the network, we can leverage this thread as the producer thread to push data downstream to MediaCodec.
   = Cons: Container parsers need to be modified so that they can accept partial data (and maybe buffer these data inside themselves) and then deliver completely parsed data downstream automatically. Coding efforts and testing efforts would be bigger than the previous proposal.

Any further thoughts on this issue will be appreciated.

	Bruce Sun [:brsun] Assignee
	Comment 10 • 11 years ago

I think there might be more easier to integrate MediaCodec into gecko with another proposal:
 - #3: Similar to #1, one dedicated pull thread is created to drive the data routing from the source to media queues. Similar to #2, the original data extractors and original MediaSource should be modified, but we simply add one read function with the timeout capability; and we route data in gecko by ourselves before passing them to MediaCodec.
   = Pros: Better than #1 because android::MediaSource::read() can be avoided in this new model. Simpler than #2 because most of the original MediaResource and MediaSource codes can be reused. We need to add our own reading functions with timeout capabilities on these modules so that we can avoid using android::MediaSource::read().
   = Cons: Unlike #1, MediaSource and MediaResource cannot be directly used in this proposal. Some new APIs are needed to be added in order to avoid using android::MediaSource::read(). But the total efforts to apply this proposal would be less than the efforts to apply #2.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 11 • 11 years ago

Nice diagrams Bruce.

I prefer something like option #2, as it completely avoid blocking reads, which are a problem with other media frameworks in desktop Firefox too.

I've been wanting to make our desktop MediaDecoder backends asynchronous like this for some time now, but have not had the time to do it. We briefly discussed doing something like this in the refactoring session at the Auckland media work week in November.

I'd like to suggest that MediaResource uses an nsIThreadPool (or an equivalent task queue) for pushing data to decode on, instead of each MediaResource having its own thread. Then we don't need decode threads either. We have a few bugs (like bug 930797 and others) on file where people (including partners) claim our media decoders create too many threads. If we can limit the number of threads for all open media elements to something like x*$number_of_cores, we can alleviate these concerns. This will make our media stack more scalable, and reduce memory usage since we won't need to reserve virtual address space for each threads' stack.

Note we need something a little more sophisticated than a raw nsIThreadPool, as we need to prevent dispatching two events to run the same decoder on different threads at the same time. All decoder tasks needs to be threadsafe as well, as they can potentially be run on a different thread every time they're called.

And we must not do blocking reads on threads in the thread pool, as we'll prevent other decoders from decoding.

Also, not creating threads and blocking on them makes it easier to shutdown decoders that aren't needed anymore (decoders for paused media in background tabs). We want to do this on desktop, and I'm sure it will be a problem on mobile too.

Ideally I'd like to see our media stack limited to about 4 threads/task queues total, and all playing media taking turns dispatching events to those task queues to decode data as more data comes in over the network. I'd also like to see us remove MediaDecoderStateMachine::DecodeLoop(), and instead just dispatch a task to decode more data when MediaDecoderStateMachine::AdvanceFrame() and AudioLoop()/lib_cubeb pop data from the MediaQueue. We'd need to add "listeners" to the MediaQueue, to listen for pushes and pops to achieve this. The push and pop handler could run on the state machine thread, or in the MediaResource thread pool perhaps.

We can't refactor MediaDecoderStateMachine to remove DecodeLoop() for all existing MediaDecoderReaders at once, it's too much work. But we can create a copy of MediaDecoderStateMachine which runs asynchronously (removing DecodeLoop(), and using the thread pool), and return that in MediaOmxDecoder::CreateStateMachine() and other backends that can use it this new mode.

It's a lot of work, but I think it's an important change to make. I'm keen to see it happen and will provide guidance and help if you need it.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 12 • 11 years ago

It might be better to read the following discussion in WebAPI ml.
https://groups.google.com/forum/#!topic/mozilla.dev.webapi/N9BW0V7MUHA

	Marco Chen [:mchen]
	Comment 13 • 11 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> It might be better to read the following discussion in WebAPI ml.
> https://groups.google.com/forum/#!topic/mozilla.dev.webapi/N9BW0V7MUHA

Hi Sotaro,

Do you paste the link to wrong bug here?
It may be related to Thumbnail Generation.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 14 • 11 years ago

(In reply to Marco Chen [:mchen] [PTO from 2013/12/24 ~ 2014/01/02] from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > It might be better to read the following discussion in WebAPI ml.
> > https://groups.google.com/forum/#!topic/mozilla.dev.webapi/N9BW0V7MUHA
> 
> Hi Sotaro,
> 
> Do you paste the link to wrong bug here?
> It may be related to Thumbnail Generation.

No, it is a correct link. MediaDecoderStateMachine is used also for thumbnail generation. Within the discussion, there are some talking about some problems about current StateMachine's problem. The link is just for FYI.

	Bruce Sun [:brsun] Assignee
	Comment 15 • 11 years ago

I found that there are some wrong arrows in the diagram #1 and #3, so I modified it a little bit just for correction in this version.

I add a proposal #4 in this attachment, which is modified from #3:
 - Modify MediaResource as a non-blocking component: Calling MediaCacheStream::Read() and RtspTrackBuffer::ReadBuffer() might be blocked if there is no data in the cache. We have to do some modification on this and to make sure there is a reading function in MediaResource that won't be blocked in any circumstances.
 - Use the decode thread to pull data into MediaCodec: The additional pull thread in #3 is a kind of waste. Since MediaCodec can be used as a non-blocking decoder component, it is ok to use the decode thread to fill input data to MediaCodec and to retrieve output data from MediaCodec. Similar to #3, data retrieval APIs of all components along the data path between MediaResource and MediaDecoderReader should be non-blocking, so it is ok to retrieve data from these components on the decode thread. Furthermore, since there would be one non-blocking function now(see previous bullet), it is ok to call this reading function on the decode thread, too.

The benefit of this proposal #4 is the simplicity of the threading model. Since every component on the decoding data flow is non-blocking, and there is only one thread to perform the decoding task, it should be easier and more flexible to be changed into task-based working model in the future.

The efforts to apply #4 would be:
 - Modify all derived classes of MediaResource to have a non-blocking reading function.
 - Modify all components on the data path between MediaResource and MediaDecoderReader to have a non-blocking reading function. All these components should be able to buffer uncertain amount of data from MediaResource and to produce demuxed output data in decodable units.
 - Wrap MediaCodec to handle media resource management.
 - Handle the data flow of MediaCodec in a newly derived MediaDecoderReader on the decode thread.

	Bruce Sun [:brsun] Assignee
	Comment 16 • 11 years ago

It is worth to note that there might be additional thread(s) introduced by MediaCodec. We need to prepare one thread (MediaCodec::mLooper) for MediaCodec during construction. If this MediaCodec is used as a video codec, there will be one additional thread (MediaCodec::mCodecLooper) created inside it.

ref: https://android.googlesource.com/platform/frameworks/av/+/master/media/libstagefright/MediaCodec.cpp

	Bruce Sun [:brsun] Assignee
	Comment 17 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #11)
> Nice diagrams Bruce.
> 
> I prefer something like option #2, as it completely avoid blocking reads,
> which are a problem with other media frameworks in desktop Firefox too.
> 
> I've been wanting to make our desktop MediaDecoder backends asynchronous
> like this for some time now, but have not had the time to do it. We briefly
> discussed doing something like this in the refactoring session at the
> Auckland media work week in November.
> 
> I'd like to suggest that MediaResource uses an nsIThreadPool (or an
> equivalent task queue) for pushing data to decode on, instead of each
> MediaResource having its own thread. Then we don't need decode threads
> either. We have a few bugs (like bug 930797 and others) on file where people
> (including partners) claim our media decoders create too many threads. If we
> can limit the number of threads for all open media elements to something
> like x*$number_of_cores, we can alleviate these concerns. This will make our
> media stack more scalable, and reduce memory usage since we won't need to
> reserve virtual address space for each threads' stack.
> 
> Note we need something a little more sophisticated than a raw nsIThreadPool,
> as we need to prevent dispatching two events to run the same decoder on
> different threads at the same time. All decoder tasks needs to be threadsafe
> as well, as they can potentially be run on a different thread every time
> they're called.
> 

Agree.

> And we must not do blocking reads on threads in the thread pool, as we'll
> prevent other decoders from decoding.
> 
> Also, not creating threads and blocking on them makes it easier to shutdown
> decoders that aren't needed anymore (decoders for paused media in background
> tabs). We want to do this on desktop, and I'm sure it will be a problem on
> mobile too.
> 
> Ideally I'd like to see our media stack limited to about 4 threads/task
> queues total, and all playing media taking turns dispatching events to those
> task queues to decode data as more data comes in over the network. I'd also
> like to see us remove MediaDecoderStateMachine::DecodeLoop(), and instead
> just dispatch a task to decode more data when
> MediaDecoderStateMachine::AdvanceFrame() and AudioLoop()/lib_cubeb pop data
> from the MediaQueue. We'd need to add "listeners" to the MediaQueue, to
> listen for pushes and pops to achieve this. The push and pop handler could
> run on the state machine thread, or in the MediaResource thread pool perhaps.
> 

May I know which kind of working model that you prefer?
 - Model 1: The data source simply tries to push data to the data sink as many as possible. MediaResource has its own thread and actively calls downstream components (may be MediaDecoderReader) to receive data by using a non-blocking function call (maybe MediaDecoderReader::NonBlockingReceive(...)).
 - Model 2: The data source can notify the data sink to retrieve data on demand when there is some data available at the data source. MediaResource has some notification mechanism (may be through a registered callback function or a posted message to some message loop) to notify the downstream component (maybe MediaDecoderReader), and the downstream component actively calls MediaResource to send data with blocking function call (maybe the original MediaResource::Read(...)).
 - Model 3: The data sink can get data from the data source on demand when there is some free space available at the data sink. MediaQueue has some notification mechanism to notify there is still some free space that can be filled, and MediaDecoderReader actively calls MediaResource to send data with non-blocking function call (may MediaResource::NonBlockRead(...)).
 - Model 4: The data sink simply tries to pull data from the data source as many as possible. The downstream component has its own thread and continuously calls MediaResource to send data with some non-blocking function call (maybe MediaResource::NonBlockRead(...)).
 - Some other models.

> We can't refactor MediaDecoderStateMachine to remove DecodeLoop() for all
> existing MediaDecoderReaders at once, it's too much work. But we can create
> a copy of MediaDecoderStateMachine which runs asynchronously (removing
> DecodeLoop(), and using the thread pool), and return that in
> MediaOmxDecoder::CreateStateMachine() and other backends that can use it
> this new mode.
> 
> It's a lot of work, but I think it's an important change to make. I'm keen
> to see it happen and will provide guidance and help if you need it.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 18 • 11 years ago

(In reply to Bruce Sun [:brsun] from comment #17)
>  - Model 3: The data sink can get data from the data source on demand when
> there is some free space available at the data sink. MediaQueue has some
> notification mechanism to notify there is still some free space that can be
> filled, and MediaDecoderReader actively calls MediaResource to send data
> with non-blocking function call (may MediaResource::NonBlockRead(...)).

I think Model 3.

I assume that by "data sink" you mean the MediaDecoderReader, and by "data source" you mean MediaResource?

I think if MediaDecoderReader can do its decoding in a thread pool using non-blocking reads, then we can be in good shape.

	Bruce Sun [:brsun] Assignee
	Comment 19 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #18)
> (In reply to Bruce Sun [:brsun] from comment #17)
> >  - Model 3: The data sink can get data from the data source on demand when
> > there is some free space available at the data sink. MediaQueue has some
> > notification mechanism to notify there is still some free space that can be
> > filled, and MediaDecoderReader actively calls MediaResource to send data
> > with non-blocking function call (may MediaResource::NonBlockRead(...)).
> 
> I think Model 3.
> 
> I assume that by "data sink" you mean the MediaDecoderReader, and by "data
> source" you mean MediaResource?
> 

Yes, that's what I mean.

> I think if MediaDecoderReader can do its decoding in a thread pool using
> non-blocking reads, then we can be in good shape.

Thanks for your suggestion. My plan is to go proposal #4 first and to separate the modification of MediaResource::NonBlockRead(...) into another bug. After these tasks are done, we can continue integrate the decoding flow into the thread pool solution. How does it sound to you?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 20 • 11 years ago

For non-blocking read you can use MediaResource::ReadFromCache():
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaResource.h#343

This will only read data that has already been downloaded. It will not block. It will mark the data as read, so the media cache may decide to evict it later, since it's already been used.

Do we need a way to signal to the MediaCache/MediaResource that we need to download/cache a specified byte range, and request a callback when that byte range is cached.

I am currently working on using thread pools only inside the MP4Reader, so that others don't have to wait for me to change MediaDecoderStateMachine to use thread pools before they can start writing PlatformDecoderModules for the MP4Reader using thread pools.

Once I have MP4Reader using thread pools, I was going to move MP4Reader's thread pools into MediaDecoderStateMachine and change MediaDecoderStateMachine so that DecodeLoop() runs on the thread pools. Changing MediaDecoderStateMachine is tricky, is a lot of work, and may take some time, so I didn't want to block people writing PlatformDecoderModules on changing MediaDecoderStateMachine.

I am working on a task queue abstraction that synchronizes events dispatched to it. This means events dispatched to a task queue run in the order in which they've been dispatched to the queue, in the thread pool, but they may run across multiple threads in a thread safe manner. I'm going to use this for async decoding in the MP4Reader backend, and MediaDecoderStateMachine will use this for running the DecodeLoop() in a thread pool.

bwu: I can attach my task queue patch to bugzilla separately, so that you can start using it in the MediaOMXReader backend before the rest of my changes have landed. Would that be helpful?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 21 • 11 years ago

(In reply to Chris Pearce (:cpearce) from comment #20)
> bwu: I can attach my task queue patch to bugzilla separately, so that you
> can start using it in the MediaOMXReader backend before the rest of my
> changes have landed. Would that be helpful?

Sorry, I meant this question for brsun, not bwu!

	Bruce Sun [:brsun] Assignee
	Comment 22 • 11 years ago

WIP: prototype of MediaCodecProxy, which uses MediaResourceHandler to communicate with IMediaResourceManagerClient and IMediaResourceManagerService.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 23 • 11 years ago

When the patches becomes stable, we need to talk to codeaurora about power consumption. IIRC, if we want to save power consumption, qcom's hw codec needs to be set explicitly to paused state. I do not know this limitation is still present to recent qcom's device. For OMXCodec, it was done by Bug 860760.

	Bruce Sun [:brsun] Assignee
	Comment 24 • 11 years ago

WIP: prototype of MediaCodecProxy, which uses MediaResourceHandler to communicate with IMediaResourceManagerClient and IMediaResourceManagerService.

p.s. Codes of MediaResourceHandler didn't exist in the previous patch. And those codes have been purged after I noticed this bad news... Re-add missing codes for MediaResourceHandler now.

	Blake Wu [:bwu][:blakewu]
	Comment 25 • 11 years ago

(In reply to Bruce Sun [:brsun] from comment #24)
> Created attachment 8382077 [details] [diff] [review]
> bug904177_create_mediacodecproxy.v2.patch
> 
> WIP: prototype of MediaCodecProxy, which uses MediaResourceHandler to
> communicate with IMediaResourceManagerClient and
> IMediaResourceManagerService.
> 
Cool! I can use this MediaCodecProxy as well in the bug 941302 :)

	Ivan Tsay (:ITsay)
	Comment 26 • 11 years ago

Move to backlog with priority whiteboard tag

	Bruce Sun [:brsun] Assignee
	Comment 27 • 10 years ago

WIP: prototype of MediaCodecProxy, MediaCodecReader, MediaCodecDecoder
 - mp3 can be playback by using android::MediaCodec.

	Ivan Tsay (:ITsay)
	Comment 28 • 10 years ago

Move this video/audio playback enhancement feature to 2.1 due to the following reasons
- Engineer's time was mostly spent on 1.3/1.4 blockers in sprint 1 and half of sprint 2
- Need more time to integrate media codec in gecko. Also need to have more unit-test on this new media codec in gecko. 
- This bug just resolves half of the enhancement works. There will be one or two more busg for enhancing the data source reading part to complete the entire enhancement. 

User impact / Partner impact:
- N/A. This enhancement feature comes from our technical evaluation on current video/audio decoding mechanism. So there won't be any impact on partner requirement if this feature is not shipped in 2.0.

	Bruce Sun [:brsun] Assignee
	Comment 29 • 10 years ago

This WIP patch can use MediaCodec to playback audio and video files.

	Bruce Sun [:brsun] Assignee
	Comment 30 • 10 years ago

Create MediaResourceHandler and MediaCodecProxy for codec resource management.

	Bruce Sun [:brsun] Assignee
	Comment 31 • 10 years ago

Create I420ColorConverterHelper for I420 color conversion.

	Bruce Sun [:brsun] Assignee
	Comment 32 • 10 years ago

Extract MediaStreamSource into one individual file.

	Bruce Sun [:brsun] Assignee
	Comment 33 • 10 years ago

Create MediaCodecReader for MediaCodecProxy.

	Bruce Sun [:brsun] Assignee
	Comment 34 • 10 years ago

Create MediaCodecDecoder for MediaCodecReader.

	Bruce Sun [:brsun] Assignee
	Comment 35 • 10 years ago

Integrate MediaCodecReader and MediaCodecDecoder into DecoderTraits.

	Bruce Sun [:brsun] Assignee
	Comment 36 • 10 years ago

Enable MediaCodec by default.

	Bruce Sun [:brsun] Assignee
	Comment 37 • 10 years ago

Known issues of MediaCodec integration currently:
 - AudioOffloadPlayer has not been integrated in MediaCodecReader and MediaCodecDecoder yet.
 - The decoding thread is still used to (blocking) read from MediaResource to get input data for MediaCodec.

	Bruce Sun [:brsun] Assignee
	Comment 38 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #37)
> Known issues of MediaCodec integration currently:
>  - AudioOffloadPlayer has not been integrated in MediaCodecReader and
> MediaCodecDecoder yet.
>  - The decoding thread is still used to (blocking) read from MediaResource
> to get input data for MediaCodec.

 - GraphicBuffer is not used. So memcpy() is used to copy each decoded audio/video frame into the corresponding MediaQueue.

	Bruce Sun [:brsun] Assignee
	Comment 39 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #38)
> (In reply to Bruce Sun [:brsun] from comment #37)
> > Known issues of MediaCodec integration currently:
> >  - AudioOffloadPlayer has not been integrated in MediaCodecReader and
> > MediaCodecDecoder yet.
> >  - The decoding thread is still used to (blocking) read from MediaResource
> > to get input data for MediaCodec.
> 
>  - GraphicBuffer is not used. So memcpy() is used to copy each decoded
> audio/video frame into the corresponding MediaQueue.

 - RTSP has not been integrated yet.

	Bruce Sun [:brsun] Assignee
	Comment 40 • 10 years ago

Although some of existing functions have not been included in these patches, I would like to have a workable infrastructure of MediaCodec at beginning. Then all the missing functions can be handled in other followup bugs once the basic codebase becomes stable.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 41 • 10 years ago

Comment on attachment 8448665 [details] [diff] [review]
Part 4: Create MediaCodecReader (v1)

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

You should be using Mozilla standard coding style for new code you write in Mozilla owned code.

Like method parameters with "a" prefix, capital letters at the start of function names, etc.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

::: content/media/omx/MediaCodecReader.cpp
@@ +188,5 @@
> +
> +  // Get one audio output data from MediaCodec
> +  CodecBufferInfo bufferInfo;
> +  TimeStamp timeout = TimeStamp::Now() + TimeDuration::FromSeconds(sMaxAudioDecodeDurationS);
> +  status_t status = GetCodecOutputData(mAudioElement, bufferInfo, sInvalidTimestampUs, timeout);

Do you get some kind of callback or notification when a decoded sample arrives? Now that bug 979104 has landed, you may be able to implement your new Reader with non-blocking decoding.

To do that, you implement MediaDecoderReader::RequestAudioData(), which returns immediately, but needs to ensure that one decoded MediaData is sent back to the state machine by calling GetCallback()->OnAudioDecoded().

Same thing for video. Ping me if you want more details.

	Bruce Sun [:brsun] Assignee
	Comment 42 • 10 years ago

Thanks Chris. I'll check mismatched coding style again and study bug 979104 for non-blocking decoding.

	Bruce Sun [:brsun] Assignee
	Comment 43 • 10 years ago

Just want to take notes. Thanks for Chris's info provided on IRC, bug 1033055 and bug 1032633 are doing non-blocking decoding as well on another reader.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 44 • 10 years ago

Comment on attachment 8448661 [details] [diff] [review]
Part 1: Create MediaCodecProxy (v1)

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

::: content/media/omx/mediaresourcemanager/MediaResourceHandler.cpp
@@ +30,5 @@
> +
> +  sp<MediaResourceManagerClient> client = new MediaResourceManagerClient(this);
> +  sp<IMediaResourceManagerService> service = client->getMediaResourceManagerService();
> +
> +  if (client == nullptr || service == nullptr) {

Do not need to check "client". "new" is always infallible in gecko.

::: content/media/omx/moz.build
@@ +45,5 @@
>      SOURCES += [
>          'RtspOmxDecoder.cpp',
>          'RtspOmxReader.cpp',
>      ]
> +    

nit, remove spece.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 45 • 10 years ago

Comment on attachment 8448663 [details] [diff] [review]
Part 2: Create I420ColorConverterHelper (v1)

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

In future, it might be better to move these converter files to gfx/layers, if they are used from GrallocImage.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 46 • 10 years ago

Comment on attachment 8448667 [details] [diff] [review]
Part 5: Create MediaCodecDecoder (v1)

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

::: content/media/omx/moz.build
@@ +47,5 @@
>      SOURCES += [
>          'RtspOmxDecoder.cpp',
>          'RtspOmxReader.cpp',
>      ]
>      

nit, remove space.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 47 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #38)
> (In reply to Bruce Sun [:brsun] from comment #37)
> > Known issues of MediaCodec integration currently:
> >  - AudioOffloadPlayer has not been integrated in MediaCodecReader and
> > MediaCodecDecoder yet.
> >  - The decoding thread is still used to (blocking) read from MediaResource
> > to get input data for MediaCodec.
> 
>  - GraphicBuffer is not used. So memcpy() is used to copy each decoded
> audio/video frame into the corresponding MediaQueue.

The following is also necessary.
- Get correct variable-bitrate mp3's track length.
   It was added to OmxDecoder by Bug 831224.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 48 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> (In reply to Bruce Sun [:brsun] from comment #38)
> > (In reply to Bruce Sun [:brsun] from comment #37)
> > > Known issues of MediaCodec integration currently:
> > >  - AudioOffloadPlayer has not been integrated in MediaCodecReader and
> > > MediaCodecDecoder yet.
> > >  - The decoding thread is still used to (blocking) read from MediaResource
> > > to get input data for MediaCodec.
> > 
> >  - GraphicBuffer is not used. So memcpy() is used to copy each decoded
> > audio/video frame into the corresponding MediaQueue.
> 
> The following is also necessary.
> - Get correct variable-bitrate mp3's track length.
>    It was added to OmxDecoder by Bug 831224.

Also need to confirm if MediaCodec reduces power consumption to acceptable level when video/audio elements are in pause state. On caf android::OMXCodec case, we needed to call OMXCodec::pause().

	Kai-Chih Hu [:khu]
	Comment 49 • 10 years ago

Looks like the target milestone is outdated. Can we have an updated one? Thanks.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 50 • 10 years ago

Comment on attachment 8448665 [details] [diff] [review]
Part 4: Create MediaCodecReader (v1)

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

It seems good as a first step, though some capabilities are missing.

::: content/media/omx/moz.build
@@ +47,5 @@
>      SOURCES += [
>          'RtspOmxDecoder.cpp',
>          'RtspOmxReader.cpp',
>      ]
>      

nit, remove space.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 51 • 10 years ago

Comment on attachment 8448670 [details] [diff] [review]
Part 7: Enable MediaCodec (v1)

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

It seems better to split this part to a different bug. It should be enabled only when MediaCodec works as a replacement of OMXCodec.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 52 • 10 years ago

I reviewed the patch from android stagefright point of view. It seems better to be reviewed from a gecko media framework point of view by other person.

	Sotaro Ikeda [:sotaro] Reporter
	Comment 53 • 10 years ago

Current missing capabilities could be added later by other bugs.

	Blake Wu [:bwu][:blakewu]
	Comment 54 • 10 years ago

Hi Bruce, 
Since PlatformDecoderModule, bug 941302, also uses MediaCodec and have a MediaCodecProxy on top of it as well, it should be better to extractor this common part as a separate bug to let this bug and bug 941302 be able to both use it. How do you think?

	Bruce Sun [:brsun] Assignee
	Comment 55 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> Current missing capabilities could be added later by other bugs.

Thanks Sotaro. I've just created a meta bug for followup tasks: bug 1033900.

	Bruce Sun [:brsun] Assignee
	Comment 56 • 10 years ago

(In reply to Blake Wu [:bwu][:blakewu] from comment #54)
> Hi Bruce, 
> Since PlatformDecoderModule, bug 941302, also uses MediaCodec and have a
> MediaCodecProxy on top of it as well, it should be better to extractor this
> common part as a separate bug to let this bug and bug 941302 be able to both
> use it. How do you think?

Hi Blake,
Good suggestion. The objective of MediaCodecProxy in my patch is trying to minimize any modification on the public interface of android::MediaCodec, and trying to integrate current resource management onto it at the same time. The objective of MediaCodecProxy on bug 941302 seems not only want to handle the resource management, but also want to replace the interface of MediaCodec with a more generic style. Suppose the interface of PlatformDecoderModule is already generic enough, how about using this minimized proxy class of MediaCodec and directly integrating it into B2G-PlatformDecoderModule?

	Bruce Sun [:brsun] Assignee
	Comment 57 • 10 years ago

Update: I'm try to figure out how to solve build errors on B2G ICS Emulator TBPL
 - https://tbpl.mozilla.org/?tree=Try&rev=3478b54c2b5e

	Bruce Sun [:brsun] Assignee
	Comment 58 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #57)
> Update: I'm try to figure out how to solve build errors on B2G ICS Emulator
> TBPL
>  - https://tbpl.mozilla.org/?tree=Try&rev=3478b54c2b5e

I will change the condition checking in moz.build from:
 - if CONFIG['ANDROID_VERSION'] >= 16:
to:
 - if int(CONFIG['ANDROID_VERSION']) >= 16:
and protect I420ColorConverterHelper.* with the above checking.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 59 • 10 years ago

Comment on attachment 8448665 [details] [diff] [review]
Part 4: Create MediaCodecReader (v1)

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

In general it looks pretty good, but please update this code to follow the Mozilla C++ coding style.

::: content/media/omx/MediaCodecReader.cpp
@@ +35,5 @@
> +using namespace android;
> +
> +namespace mozilla {
> +
> +namespace {

You don't need the "namespace {" do you? It seems unnecessary.

@@ +104,5 @@
> +  }
> +}
> +
> +MediaCodecReader::MediaElement::MediaElement()
> +: mDurationUs(0)

Indent parameters 2 spaces, here and in other constructors.

@@ +136,5 @@
> +}
> +
> +MediaCodecReader::MediaCodecReader(AbstractMediaDecoder* aDecoder)
> +: MediaDecoderReader(aDecoder)
> +, mHandler(new MessageHandler(MOZ_THIS_IN_INITIALIZER_LIST()))

If you're going to use MOZ_THIS_IN_INITIALIZER_LIST() in the constructor, you should make the class MOZ_FINAL, otherwise the this pointer you pass in may change in future in unexpected ways.

@@ +148,5 @@
> +MediaCodecReader::~MediaCodecReader()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
> +
> +  ReleaseResources();

Please do your shutdown in a MediaDecoderReader::Shutdown() override function. This gets called when the decode task queue is being shutdown [1], and the reader should not have any calls into it from the MediaDecoderStateMachine after Shutdown() has run. If parts of your reader need to be shutdown on the main thread (or some other thread for that matter), you can dispatch Runnables to those threads to do the shutdown on those threads.

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#2247

@@ +174,5 @@
> +}
> +
> +void MediaCodecReader::ReleaseDecoder()
> +{
> +  // TODO: NO-OP?

If this is a no-op, remove it!

@@ +192,5 @@
> +  status_t status = GetCodecOutputData(mAudioElement, bufferInfo, sInvalidTimestampUs, timeout);
> +  if (status == -EAGAIN) {
> +    // Try it again later.
> +    return true;
> +  }

if (status == -EAGAIN) {
  // ...
} else {
  // ...
}

(else on the same line as } and {, here and elsewhere in your code.)

@@ +232,5 @@
> +  }
> +
> +  int64_t threshold = sInvalidTimestampUs;
> +  if (aKeyframeSkip && IsValidTimestampUs(aTimeThreshold)) {
> +    threshold = aTimeThreshold;

You need to set aKeyframeSkip=false once the decode has skipped up to the next keyframe, otherwise the state machine will always tell you to skip to the next keyframe. I'm guessing here is where you know whether you should have skipped up to the next keyframe?

@@ +242,5 @@
> +  status_t status = GetCodecOutputData(mVideoElement, bufferInfo, threshold, timeout);
> +  if (status == -EAGAIN) {
> +    // Try it again later.
> +    return true;
> +  }

} else if (...) {

@@ +314,5 @@
> +      // was reported relative to the picture size reported by the container.
> +      picture.x = (mVideoElement.mPictureRect.x * mVideoElement.mWidth) / mVideoElement.mFrameSize.width;
> +      picture.y = (mVideoElement.mPictureRect.y * mVideoElement.mHeight) / mVideoElement.mFrameSize.height;
> +      picture.width = (mVideoElement.mWidth * mVideoElement.mPictureRect.width) / mVideoElement.mFrameSize.width;
> +      picture.height = (mVideoElement.mHeight * mVideoElement.mPictureRect.height) / mVideoElement.mFrameSize.height;

You should validate the video region again here using IsValidVideoRegion() [1], to ensure that you're not going to try to allocate a videoframe too large to fit in memory, or otherwise invalid.

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#174

@@ +406,5 @@
> +
> +  return NS_OK;
> +}
> +
> +nsresult MediaCodecReader::Seek(int64_t aTime,

To follow our coding style, this should be indented like so:

nsresult
MediaCodecReader::Seek(int64_t aTime,
                       int64_t aStartTime,
                       int64_t aEndTime,
                       int64_t aCurrentTime)
{
  // ...
}

(My comments in Splinter are a non-fixed width font, so it's hard to see, but the method arguments should line up with the "(" on the function, and the return type should be on its own line.)

@@ +413,5 @@
> +                      int64_t aCurrentTime)
> +{
> +  MOZ_ASSERT(mDecoder->OnDecodeThread(), "Should be on decode thread.");
> +
> +  ResetDecode();

You don't need to call ResetDecode(), it happens already in the caller; MediaDecoderStateMachine::DecodeSeek().

@@ +448,5 @@
> +        mVideoElement.mSeekTimeUs = timestamp;
> +      }
> +      format = nullptr;
> +    }
> +    track_buffer->release();

Does this mean that you discard the first frame after seek? That means if someone is seeking exactly to the keyframe, this will discard the keyframe and the seek will be off by one frame?

@@ +473,5 @@
> +  ReleaseResources();
> +  return false;
> +}
> +
> +void MediaCodecReader::ReleaseCriticalResources()

Why not just call this ReleaseMediaResources()?

@@ +573,5 @@
> +    return false;
> +  }
> +
> +  sp<MetaData> extractorMetaData = mExtractor->getMetaData();
> +  // TODO: NO-OP?

If it's a no-op, remove it. :)

@@ +589,5 @@
> +    }
> +
> +    if (audioTrackIndex == invalidTrackIndex &&
> +        !strncasecmp(mime, "audio/", 6)) {
> +      audioTrackIndex = i;

You also need to validate that the streams are codec types that we want to play. So aac/mp3 for audio, and H.264 for video. And add exceptions as appropriate for privileged/certified apps.

@@ +603,5 @@
> +    NS_WARNING("OMX decoder could not find audio or video tracks");
> +    return false;
> +  }
> +
> +  mDecoder->GetResource()->SetReadMode(MediaCacheStream::MODE_PLAYBACK);

This happens already in MediaDecoderReader::DecodeMetadata(), if ReadMetadata() succeeds. Why do you need it earlier here?

@@ +841,5 @@
> +  }
> +
> +  // read video metadata from MediaCodec
> +  sp<AMessage> videoCodecFormat;
> +  if (mVideoElement.mCodec->getOutputFormat(&videoCodecFormat) == OK && videoCodecFormat != nullptr) {

if (mVideoElement.mCodec->getOutputFormat(&videoCodecFormat) != OK || !videoCodecFormat) {
  return false;
}

Then you don't need the indentation below, making the code easier to read.

@@ +880,5 @@
> +    int32_t display_width = crop_right - crop_left + 1;
> +    int32_t display_height = crop_bottom - crop_top + 1;
> +    nsIntRect picture_rect(0, 0, mVideoElement.mWidth, mVideoElement.mHeight);
> +    nsIntSize display_size(display_width, display_height);
> +    if (!IsValidVideoRegion(mVideoElement.mFrameSize, picture_rect, display_size)) {

Ah, here is where you're validating the video region.

Do the videos linked to in bug 823646 comment 1 display properly with your reader? In those videos the video frame size differs to the container, IIRC. If the frame size changes, you need to re-validate the video region.

@@ +936,5 @@
> +    }
> +
> +    // read() fails
> +    if (status == INFO_FORMAT_CHANGED) {
> +      return INFO_FORMAT_CHANGED;

Can you not reconfigure and re-validate the decode pipeline/reader and continue outputting the new format?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 60 • 10 years ago

Comment on attachment 8448667 [details] [diff] [review]
Part 5: Create MediaCodecDecoder (v1)

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

r+ with Sotaro's comment addressed.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 61 • 10 years ago

Comment on attachment 8448668 [details] [diff] [review]
Part 6: Integrate DecoderTraits (v1)

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

This moz.build changes should really be in the "Part 1" patch.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 62 • 10 years ago

Comment on attachment 8448664 [details] [diff] [review]
Part 3: Extract MediaStreamSource (v1)

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

r=cpearce with my comments below addressed.

::: content/media/omx/MediaStreamSource.cpp
@@ +11,5 @@
> +#include "nsISeekableStream.h"
> +
> +namespace android {
> +
> +MediaStreamSource::MediaStreamSource(MediaResource *aResource,

MediaStreamSource::MediaStreamSource(MediaResource *aResource,
                                     AbstractMediaDecoder *aDecoder)
  : mResource(aResource)
  , mDecoder(aDecoder)
{
}

@@ +31,5 @@
> +{
> +  char *ptr = static_cast<char *>(data);
> +  size_t todo = size;
> +  while (todo > 0) {
> +    Mutex::Autolock autoLock(mLock);

mResource->ReadAt(...). This does the lock/seek for you. Then you don't need the (extra) lock.

::: content/media/omx/MediaStreamSource.h
@@ +24,5 @@
> +  typedef mozilla::AbstractMediaDecoder AbstractMediaDecoder;
> +
> +  Mutex mLock;
> +  nsRefPtr<MediaResource> mResource;
> +  AbstractMediaDecoder *mDecoder;

mDecoder looks unused? If it is, please remove it.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 63 • 10 years ago

Comment on attachment 8448661 [details] [diff] [review]
Part 1: Create MediaCodecProxy (v1)

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

Please make this consistent with Mozilla C++ coding style before landing.

::: content/media/omx/MediaCodecProxy.cpp
@@ +27,5 @@
> +
> +MediaCodecProxy::MediaCodecProxy(
> +    sp<ALooper> looper, const char *mime, bool encoder,
> +    bool async, wp<CodecResourceListener> listener)
> +: mCodecLooper(looper)

Indent initializers.

@@ +119,5 @@
> +  }
> +
> +  while (codec.promote() != nullptr) {
> +    // this value come from stagefrigh's AwesomePlayer.
> +    usleep(1000);

Yuck. Is there no way you can get a callback or something when the codec is released?

@@ +131,5 @@
> +
> +  return mCodec != nullptr;
> +}
> +
> +status_t MediaCodecProxy::configure(

To follow our coding style, this should be formatted/styled like so:

status_t
MediaCodecProxy::configure(const sp<AMessage> &aFormat,
                           const sp<Surface> &aNativeWindow,
                           const sp<ICrypto> &aCrypto,
                           uint32_t flags)
{
  // ....
}

@@ +306,5 @@
> +  }
> +  return mCodec->getOutputFormat(format);
> +}
> +
> +status_t MediaCodecProxy::getInputBuffers(Vector<sp<ABuffer> > *buffers) const

Vector<sp<ABuffer>> should work in all the compilers that we support.

::: content/media/omx/mediaresourcemanager/MediaResourceHandler.cpp
@@ +8,5 @@
> +
> +namespace android {
> +
> +MediaResourceHandler::MediaResourceHandler(const wp<ResourceListener> &listener)
> +: mListener(listener)

Indent initializer list by 2 spaces.

@@ +78,5 @@
> +  }
> +
> +  if (mState == MediaResourceManagerClient::CLIENT_STATE_RESOURCE_ASSIGNED) {
> +    listener->resourceReserved();
> +  }

} else  {

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 64 • 10 years ago

Comment on attachment 8448663 [details] [diff] [review]
Part 2: Create I420ColorConverterHelper (v1)

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

Please make sure this conforms to Mozilla C++ coding style before landing.

::: content/media/omx/I420ColorConverterHelper.cpp
@@ +31,5 @@
> +}
> +
> +I420ColorConverterHelper::~I420ColorConverterHelper()
> +{
> +  {

You don't need the extra scope {} here.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 65 • 10 years ago

Comment on attachment 8448670 [details] [diff] [review]
Part 7: Enable MediaCodec (v1)

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

I agree with Sotaro.

	Bruce Sun [:brsun] Assignee
	Comment 66 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #63)
> Comment on attachment 8448661 [details] [diff] [review]
> Part 1: Create MediaCodecProxy (v1)
> 
> Review of attachment 8448661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +119,5 @@
> > +  }
> > +
> > +  while (codec.promote() != nullptr) {
> > +    // this value come from stagefrigh's AwesomePlayer.
> > +    usleep(1000);
> 
> Yuck. Is there no way you can get a callback or something when the codec is
> released?
> 

I've create a followup bug for this: bug 1036376.

	Bruce Sun [:brsun] Assignee
	Comment 67 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #62)
> Comment on attachment 8448664 [details] [diff] [review]
> Part 3: Extract MediaStreamSource (v1)
> 
> Review of attachment 8448664 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +31,5 @@
> > +{
> > +  char *ptr = static_cast<char *>(data);
> > +  size_t todo = size;
> > +  while (todo > 0) {
> > +    Mutex::Autolock autoLock(mLock);
> 
> mResource->ReadAt(...). This does the lock/seek for you. Then you don't need
> the (extra) lock.
> 

I've create one followup bug for this: bug 1036239.

	Bruce Sun [:brsun] Assignee
	Comment 68 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #59)
> Comment on attachment 8448665 [details] [diff] [review]
> Part 4: Create MediaCodecReader (v1)
> 
> Review of attachment 8448665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general it looks pretty good, but please update this code to follow the
> Mozilla C++ coding style.
> 
> ::: content/media/omx/MediaCodecReader.cpp
> @@ +448,5 @@
> > +        mVideoElement.mSeekTimeUs = timestamp;
> > +      }
> > +      format = nullptr;
> > +    }
> > +    track_buffer->release();
> 
> Does this mean that you discard the first frame after seek? That means if
> someone is seeking exactly to the keyframe, this will discard the keyframe
> and the seek will be off by one frame?
> 

No. This data will be read back in MediaCodecReader::FillCodecInputData() if mSeekTimeUs has been properly set.

> @@ +473,5 @@
> > +  ReleaseResources();
> > +  return false;
> > +}
> > +
> > +void MediaCodecReader::ReleaseCriticalResources()
> 
> Why not just call this ReleaseMediaResources()?
> 

Just want to separate the public interface and the private worker functions.

> @@ +573,5 @@
> > +    return false;
> > +  }
> > +
> > +  sp<MetaData> extractorMetaData = mExtractor->getMetaData();
> > +  // TODO: NO-OP?
> 
> If it's a no-op, remove it. :)
> 

It will be used to check the file type for MP3 stuff: bug 1033915.

> @@ +589,5 @@
> > +    }
> > +
> > +    if (audioTrackIndex == invalidTrackIndex &&
> > +        !strncasecmp(mime, "audio/", 6)) {
> > +      audioTrackIndex = i;
> 
> You also need to validate that the streams are codec types that we want to
> play. So aac/mp3 for audio, and H.264 for video. And add exceptions as
> appropriate for privileged/certified apps.
> 

I've create one followup bug for this: bug 1036343.

> @@ +603,5 @@
> > +    NS_WARNING("OMX decoder could not find audio or video tracks");
> > +    return false;
> > +  }
> > +
> > +  mDecoder->GetResource()->SetReadMode(MediaCacheStream::MODE_PLAYBACK);
> 
> This happens already in MediaDecoderReader::DecodeMetadata(), if
> ReadMetadata() succeeds. Why do you need it earlier here?
> 

No special purpose. Just copy the calling sequence from MediaOmxReader. Will remove it.

> @@ +880,5 @@
> > +    int32_t display_width = crop_right - crop_left + 1;
> > +    int32_t display_height = crop_bottom - crop_top + 1;
> > +    nsIntRect picture_rect(0, 0, mVideoElement.mWidth, mVideoElement.mHeight);
> > +    nsIntSize display_size(display_width, display_height);
> > +    if (!IsValidVideoRegion(mVideoElement.mFrameSize, picture_rect, display_size)) {
> 
> Ah, here is where you're validating the video region.
> 
> Do the videos linked to in bug 823646 comment 1 display properly with your
> reader? In those videos the video frame size differs to the container, IIRC.
> If the frame size changes, you need to re-validate the video region.
> 

Validation is already inside UpdateVideoInfo(), which should be called when INFO_FORMAT_CHANGED occurs.

> @@ +936,5 @@
> > +    }
> > +
> > +    // read() fails
> > +    if (status == INFO_FORMAT_CHANGED) {
> > +      return INFO_FORMAT_CHANGED;
> 
> Can you not reconfigure and re-validate the decode pipeline/reader and
> continue outputting the new format?

I'll change my code to call UpdateAudioInfo() and UpdateVideoInfo() again when INFO_FORMAT_CHANGED has been encountered.

	Bruce Sun [:brsun] Assignee
	Comment 70 • 10 years ago

Comment on attachment 8448670 [details] [diff] [review]
Part 7: Enable MediaCodec (v1)

This patch should be handled on bug 1033935.

	Bruce Sun [:brsun] Assignee
	Comment 71 • 10 years ago

Part 1 is going to be handled on bug 1037282.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 72 • 10 years ago

Comment on attachment 8453026 [details] [diff] [review]
Part 4: Create MediaCodecReader (v2)

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

r=cpearce with nits addressed.

Does this patch mochitests on tbpl?

::: content/media/omx/MediaCodecReader.cpp
@@ +402,5 @@
> +    mDecoder->SetMediaDuration(duration);
> +  }
> +
> +  // Check the MediaExtractor flag if the source is seekable.
> +  mDecoder->SetMediaSeekable(IsMediaSeekable());

You don't need to call MediaDecoder::SetMediaSeekable() any more. The State Machine calls IsMediaSeekable() after it's called ReadMetadata() and does this for you.

@@ +499,5 @@
> +  if (videoframe != nullptr) {
> +    videoframe->ClearCurrentFrame();
> +  }
> +
> +  // TODO

// TODO .... what exactly? Complete this comment, or remove it and file a bug.

@@ +796,5 @@
> +  sp<MetaData> audioTrackFormat = mAudioElement.mTrack->getFormat();
> +  if (audioTrackFormat == nullptr) {
> +    return false;
> +  }
> +  // TODO: NO-OP?

What is this comment here for exactly?

::: content/media/omx/MediaCodecReader.h
@@ +46,5 @@
> +  virtual void ReleaseMediaResources();
> +
> +  // Destroys the decoding state. The reader cannot be made usable again.
> +  // This is different from ReleaseMediaResources() as it is irreversable,
> +  // whereas ReleaseMediaResources() is.

It would be clearer to say:

// This is different from ReleaseMediaResources() as Shutdown() is irreversible,
// whereas ReleaseMediaResources() is reversible.

The word "it" in English is very easy to misinterpret, it's not a good idea to use "it" in sentences where there are more than one noun involved.

@@ +82,5 @@
> +
> +  virtual bool IsMediaSeekable() MOZ_OVERRIDE;
> +
> +protected:
> +  struct MediaElement

"MediaElement" is not a great name for this, since we already have an HTMLMediaElement, and this does not represent that. Is there a better name you could use?

	Blake Wu [:bwu][:blakewu]
	Comment 73 • 10 years ago

Bruce, 
For your reference, AHandlerReflector (http://androidxref.com/4.3_r2.1/xref/frameworks/av/include/media/stagefright/foundation/AHandlerReflector.h) could play the same role as your "MessageHandler". Perhaps it can help make codes simpler.

	Bruce Sun [:brsun] Assignee
	Comment 74 • 10 years ago

Hi Blake,
Thanks for your information. The target of AHandlerReflector needs to be able to be fit into android::wp<T>, which means the target should be inherited from android::RefBase and the reference counting of it cannot be doubly managed by gecko. By adding an intermediate class MessageHandler, we can manage the reference counting of MediaCodecReader by gecko, and let MessageHandler be handled by android::RefBase.

	Bruce Sun [:brsun] Assignee
	Comment 75 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #72)
> Comment on attachment 8453026 [details] [diff] [review]
> Part 4: Create MediaCodecReader (v2)
> 
> Review of attachment 8453026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce with nits addressed.
> 
> Does this patch mochitests on tbpl?
> 

Not yet for this patch (v2). TBPL results for previous patch (v1):
 - https://tbpl.mozilla.org/?tree=Try&rev=b6e83c76e2b5

> ::: content/media/omx/MediaCodecReader.cpp
> @@ +402,5 @@
> > +    mDecoder->SetMediaDuration(duration);
> > +  }
> > +
> > +  // Check the MediaExtractor flag if the source is seekable.
> > +  mDecoder->SetMediaSeekable(IsMediaSeekable());
> 
> You don't need to call MediaDecoder::SetMediaSeekable() any more. The State
> Machine calls IsMediaSeekable() after it's called ReadMetadata() and does
> this for you.
> 

Thanks. I'll do the corresponding modification.

> @@ +499,5 @@
> > +  if (videoframe != nullptr) {
> > +    videoframe->ClearCurrentFrame();
> > +  }
> > +
> > +  // TODO
> 
> // TODO .... what exactly? Complete this comment, or remove it and file a
> bug.
> 

Thanks. I'll remove it.

> @@ +796,5 @@
> > +  sp<MetaData> audioTrackFormat = mAudioElement.mTrack->getFormat();
> > +  if (audioTrackFormat == nullptr) {
> > +    return false;
> > +  }
> > +  // TODO: NO-OP?
> 
> What is this comment here for exactly?
> 

Thanks. I'll remove it.

> ::: content/media/omx/MediaCodecReader.h
> @@ +46,5 @@
> > +  virtual void ReleaseMediaResources();
> > +
> > +  // Destroys the decoding state. The reader cannot be made usable again.
> > +  // This is different from ReleaseMediaResources() as it is irreversable,
> > +  // whereas ReleaseMediaResources() is.
> 
> It would be clearer to say:
> 
> // This is different from ReleaseMediaResources() as Shutdown() is
> irreversible,
> // whereas ReleaseMediaResources() is reversible.
> 
> The word "it" in English is very easy to misinterpret, it's not a good idea
> to use "it" in sentences where there are more than one noun involved.
> 

Thanks. That comment was copied from:
 - http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderReader.h?from=MediaDecoderReader&case=true#55
I'll do the corresponding modification.

> @@ +82,5 @@
> > +
> > +  virtual bool IsMediaSeekable() MOZ_OVERRIDE;
> > +
> > +protected:
> > +  struct MediaElement
> 
> "MediaElement" is not a great name for this, since we already have an
> HTMLMediaElement, and this does not represent that. Is there a better name
> you could use?

Thanks. I definitely should change the naming of MediaElement to avoid confusion. How about TrackElement or track something?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 76 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #75)
> (In reply to Chris Pearce (:cpearce) from comment #72)
> > "MediaElement" is not a great name for this, since we already have an
> > HTMLMediaElement, and this does not represent that. Is there a better name
> > you could use?
> 
> Thanks. I definitely should change the naming of MediaElement to avoid
> confusion. How about TrackElement or track something?

How about just Track? Then you'll have AudioTrack and VideoTrack subclasses.

	Bruce Sun [:brsun] Assignee
	Comment 77 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #76)
> How about just Track? Then you'll have AudioTrack and VideoTrack subclasses.

Sounds good. I'll do it.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 78 • 10 years ago

Comment on attachment 8453026 [details] [diff] [review]
Part 4: Create MediaCodecReader (v2)

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

r=cpearce with nits addressed. (sorry, forgot to change the flag when I made my previous comment)

	Bruce Sun [:brsun] Assignee
	Comment 79 • 10 years ago

Comment on attachment 8448661 [details] [diff] [review]
Part 1: Create MediaCodecProxy (v1)

This patch is handled on bug 1037282.

	Bruce Sun [:brsun] Assignee
	Comment 80 • 10 years ago

Since the original part 1 and part 7 are handled on separated bugs, there will be only 5 parts for this bug.

	Bruce Sun [:brsun] Assignee
	Comment 86 • 10 years ago

TBPL results: https://tbpl.mozilla.org/?tree=Try&rev=80eb8cf86ec0

	Carsten Book [:Tomcat]
	Comment 87 • 10 years ago

Hi Bruce, 

part 1-4 are done and did the checkin, part 5 failed with:

applying bug904177_part5_media_codec_traits.checkin.patch
patching file content/media/DecoderTraits.cpp
Hunk #1 succeeded at 39 with fuzz 2 (offset 0 lines).
Hunk #3 FAILED at 631
1 out of 3 hunks FAILED -- saving rejects to file content/media/DecoderTraits.cpp.rej
patching file content/media/MediaDecoder.cpp
Hunk #1 FAILED at 1711
1 out of 1 hunks FAILED -- saving rejects to file content/media/MediaDecoder.cpp.rej
patching file content/media/MediaDecoder.h
Hunk #1 FAILED at 867
1 out of 1 hunks FAILED -- saving rejects to file content/media/MediaDecoder.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug904177_part5_media_codec_traits.checkin.patch

could you take a look ? Thanks

	Carsten Book [:Tomcat]
	Comment 88 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b26a1d2a1b0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60ff172ed7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de3edf0c698
https://hg.mozilla.org/integration/mozilla-inbound/rev/40a5725d7f92

	Bruce Sun [:brsun] Assignee
	Comment 89 • 10 years ago

Update part 5 for resolving conflicts on inbound.

	Bruce Sun [:brsun] Assignee
	Comment 90 • 10 years ago

Hi Carsten,

Thanks for informing the conflict. An updated patch for part 5 has been attached as attachment 8457859 [details] [diff] [review].

	Carsten Book [:Tomcat]
	Comment 91 • 10 years ago

(In reply to Bruce Sun [:brsun] from comment #90)
> Hi Carsten,
> 
> Thanks for informing the conflict. An updated patch for part 5 has been
> attached as attachment 8457859 [details] [diff] [review].

cool, thanks - thats done now - https://hg.mozilla.org/integration/mozilla-inbound/rev/9747c285620e

	Carsten Book [:Tomcat]
	Comment 92 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b26a1d2a1b0c
https://hg.mozilla.org/mozilla-central/rev/a60ff172ed7d
https://hg.mozilla.org/mozilla-central/rev/3de3edf0c698
https://hg.mozilla.org/mozilla-central/rev/40a5725d7f92

	Ryan VanderMeulen [:RyanVM]
	Comment 93 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9747c285620e