1033912 - Use separated MediaTaskQueue for audio and video decoding in MediaCodecReader.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bruce Sun [:brsun]]

This is a followup bug of bug 904177. Audio decoding and video decoding currently share the same thread. It is better to separate them into different threads by using different MediaTaskQueue to avoid interference.

Comment 1

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

Is this in relation to a specific MediaDecoderReader? Because only MediaDecoderReaders operating in asynchronous mode could possibly decode the audio and video on separate task queues.

Comment 2

[Bruce Sun [:brsun]]

(In reply to Chris Pearce (:cpearce) from comment #1)
> Is this in relation to a specific MediaDecoderReader? Because only
> MediaDecoderReaders operating in asynchronous mode could possibly decode the
> audio and video on separate task queues.

Thanks. I've refined the title.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v01.patch

In this patch, I try to put audio/video decoding to each separated MediaTaskQueues and some code flow change.

1. new MediaTaskQueue for audio/video decoding.
2. implement RequestVideoData, RequestAudioData
3. extract FillCodecInputData from GetCodecOutputData
4. refine the seek funtionality (remove the extra decoding operation)
5. add RemovePendingTasks function in MediaTaskQueue

Hi Chris:
Is that ok if I want a RemovePendingTasks function in MediaTaskQueue?

Comment 4

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

(In reply to Benjamin Chen [:bechen] from comment #3)
> Is that ok if I want a RemovePendingTasks function in MediaTaskQueue?

Why can't you call Flush() instead?

Comment 5

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

Comment on attachment 8466946 [details] [diff] [review]
bug-1033912.v01.patch

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

Looks promising. :)

::: content/media/omx/MediaCodecReader.cpp
@@ +253,2 @@
>  bool
>  MediaCodecReader::DecodeAudioData()

DecodeAudioData should not be called if you implement RequestAudioData. So you don't need it anymore.

@@ +547,4 @@
>  }
>  
>  bool
> +MediaCodecReader::DecodeVideoFrame(bool &aKeyframeSkip, int64_t aTimeThreshold)

DecodeVideoFrame should not be called if you implement RequestVideoData. So you don't need it anymore.

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

MediaDecoderStateMachine calls ResetDecode() before calling Seek(). In ResetDecode(), you need to make sure that all requests for audio/video are cancelled. You don't want to return audio/video samples from before the seek to be returned after the seek completes.

So you probably want to flush the audio and video task queues and media queues in ResetDecode.

Comment 6

[Benjamin Chen [:bechen]]

(In reply to Chris Pearce (:cpearce) from comment #4)
> (In reply to Benjamin Chen [:bechen] from comment #3)
> > Is that ok if I want a RemovePendingTasks function in MediaTaskQueue?
> 
> Why can't you call Flush() instead?

Because I don't want the "decode thread" be blocked in Flush() function waiting for the current running task.
Once the running task is blocked somehow, the decoding thread will be blocked too.

But since the MediaDecoderStateMachine doesn't hold the monitor to call mReader->seek, maybe I should just use Flush().
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp?from=mediadecoderstatemachine.cpp#2068

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 8466946 [details] [diff] [review]
bug-1033912.v01.patch

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

MediaCodec dequeue/enqueue buffers code looks good to me. Reader/TaskQueue code needs others' help.

Comment 8

[Bruce Sun [:brsun]]

Comment on attachment 8466946 [details] [diff] [review]
bug-1033912.v01.patch

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

Looks good. But some of our original intentions to separate threads are not fulfilled.

::: content/media/omx/MediaCodecReader.cpp
@@ +260,5 @@
> +bool
> +MediaCodecReader::DecodeAudioDataSync()
> +{
> +  if (mAudioTrack.mCodec == nullptr || !mAudioTrack.mCodec->allocated()
> +      || mAudioTrack.mOutputEndOfStream) {

nit: Break long conditions after && and || logical connectives.

@@ +277,5 @@
>      status = GetCodecOutputData(mAudioTrack, bufferInfo, sInvalidTimestampUs, timeout);
>      if (status == OK || status == ERROR_END_OF_STREAM) {
>        break;
>      } else if (status == -EAGAIN) {
> +      continue; // Try it again now.

If GetCodecOutputData() always returns -EAGAIN for some reason, this while() loop cannot breaks or returns. As a result, mAudioTrack.mTaskQueue cannot be shutdown during this time period.

Probably dispatching another task and leave this while() loop after timeout could ease this condition.

@@ +432,3 @@
>        break;
>      } else if (status == -EAGAIN) {
> +      continue; // Try it again now.

If GetCodecOutputData() always returns -EAGAIN for some reason, this while() loop cannot breaks or returns. As a result, mVideoTrack.mTaskQueue cannot be shutdown during this time period.

Probably dispatching another task and leave this while() loop after timeout could ease this condition.

@@ +793,5 @@
>    mVideoTrack.mSource = nullptr;
>  }
>  
>  bool
> +MediaCodecReader::CreateTaskQueues()

There are no corresponding codes to shutdown or to cleanup mAudioTrack.mTaskQueue and mVideoTrack.mTaskQueue. Those logic should be added correspondingly.

@@ +795,5 @@
>  
>  bool
> +MediaCodecReader::CreateTaskQueues()
> +{
> +  if (mAudioTrack.mSource != nullptr && mAudioTrack.mCodec != nullptr) {

Better to check whether mAudioTrack.mTaskQueue has been allocated already or not before allocating a new one.

@@ +800,5 @@
> +    mAudioTrack.mTaskQueue = new MediaTaskQueue(
> +      SharedThreadPool::Get(NS_LITERAL_CSTRING("MediaCodecReader Audio"), 1));
> +    NS_ENSURE_TRUE(mAudioTrack.mTaskQueue, false);
> +  }
> + if (mVideoTrack.mSource != nullptr && mVideoTrack.mCodec != nullptr) {

Better to check whether mVideoTrack.mTaskQueue has been allocated already or not before allocating a new one.

Comment 9

[Bruce Sun [:brsun]]

Just want to take a note, it seems the attachment 8466946 [details] [diff] [review] is based on the attachment 8466919 [details] [diff] [review] from bug 1043900 instead of the latest master.

Comment 10

[Bruce Sun [:brsun]]

Comment on attachment 8466946 [details] [diff] [review]
bug-1033912.v01.patch

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

::: content/media/omx/MediaCodecReader.cpp
@@ +237,5 @@
> +MediaCodecReader::RequestVideoData(bool aSkipToNextKeyframe,
> +                                   int64_t aTimeThreshold)
> +{
> +  MOZ_ASSERT(GetTaskQueue()->IsCurrentThreadIn());
> +  MOZ_ASSERT(HasVideo());

Just curious. Is there any special purpose to add MOZ_ASSERT(HasVideo()) in RequestVideoData() but not to add MOZ_ASSERT(HasAudio()) in RequestAudioData()?

Comment 11

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v02.patch

Address the comments.
1. implement ResetDecode function
2. DecodeVideoFrameSync and DecodeAudioDataSync will dispatch a new task to current thread if timeouts.
3. cleanup the MediaTaskQueues

Comment 12

[Benjamin Chen [:bechen]]

Attachment: bug-1033912-part2-nits.patch

Comment 13

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

Comment on attachment 8469936 [details] [diff] [review]
bug-1033912.v02.patch

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

This looks fine apart from the Seek() function. Please fix and re-request review from me.

Bruce wrote this code, so he understands it best, so I want him to review it too.

::: content/media/omx/MediaCodecReader.cpp
@@ +645,3 @@
>      }
> +    EnableSelfDispatch(mVideoTrack);
> +    mVideoTrack.mTaskQueue->Flush();

ResetDecode() is called by the StateMachine before calling Seek(). Your ResetDecode() implementation already calls mVideoTrack.mTaskQueue->Flush(), so you should not need to call it again here.

You could assert that it was called here if you want to be safe.

@@ +653,5 @@
>    }
> +  if (HasAudio() && mAudioTrack.mSource != nullptr) {
> +    EnableSelfDispatch(mAudioTrack);
> +    mAudioTrack.mTaskQueue->Flush();
> +    mAudioTrack.mSeekTimeUs = aTime;

You actually want to seek the audio stream to the timestamp at which the video stream seeks too.

If we're doing a fastSeek(), which the B2G video controls do, we want to seek the video stream to the keyframe before the seek target. We therefore need to seek the audio stream to the same point.

So you need to wait here until the video decode completes, and then get the timestamp of the first frame, and then seek the audio stream to that time.

Comment 14

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

Comment on attachment 8469937 [details] [diff] [review]
bug-1033912-part2-nits.patch

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

::: content/media/omx/MediaCodecReader.h
@@ +26,5 @@
>  struct MOZ_EXPORT MediaSource;
>  struct MediaCodec;
>  } // namespace android
>  
> +using namespace android;

Do not put using directives in .h files, as that means anything included after it could end up using the wrong thing because of it.


'No "using" statements are allowed in header files, except inside class definitions or functions. (We don't want to pollute the global scope of compilation units that use the header file.)'

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

Comment 15

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v03.patch

Refine the Seek() function, remove SeekAudioTask SeekVideoTask.

Comment 16

[Benjamin Chen [:bechen]]

Attachment: bug-1033912-part2-nits.patch

Comment 17

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

Comment on attachment 8470749 [details] [diff] [review]
bug-1033912.v03.patch

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

r=cpearce with the small change I proposed. You must get r+ from Bruce before you land.

::: content/media/omx/MediaCodecReader.cpp
@@ +357,5 @@
> +      mAudioTrack.mDiscontinuity = false;
> +    }
> +    GetCallback()->OnAudioDecoded(a);
> +  }
> +  if (AudioQueue().IsFinished()) {

You should call AtEndOfStream() instead of IsFinished() here, otherwise you will report EOS before the last sample has been returned.

@@ +599,5 @@
> +      mVideoTrack.mDiscontinuity = false;
> +    }
> +    GetCallback()->OnVideoDecoded(v);
> +  }
> +  if (VideoQueue().IsFinished()) {

VideoQueue().AtEndOfStream()

Comment 18

[Bruce Sun [:brsun]]

Comment on attachment 8470749 [details] [diff] [review]
bug-1033912.v03.patch

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

Looks great. Just want to double-check for some further information.

::: content/media/omx/MediaCodecReader.cpp
@@ +298,5 @@
>      status = GetCodecOutputData(mAudioTrack, bufferInfo, sInvalidTimestampUs, timeout);
>      if (status == OK || status == ERROR_END_OF_STREAM) {
> +      if (bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> +        AudioQueue().Finish();
> +      }

Do we need to handle the case that |status == ERROR_END_OF_STREAM| is true but |bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS| is zero?

@@ +352,5 @@
> +  bool result = DecodeAudioDataSync();
> +  if (AudioQueue().GetSize() > 0) {
> +    AudioData* a = AudioQueue().PopFront();
> +    if (mAudioTrack.mDiscontinuity) {
> +      a->mDiscontinuity = true;

Since AudioQueue() is a public member function of MediaDecoderReader, and there is no guarantee (no comments or protected logics) that AudioQueue().PopFront() can always returns valid AudioData pointer if previous AudioQueue().GetSize() returns a value larger than zero (even on the same thread). Extra nullptr checking would be needed for |a|.

@@ +483,5 @@
> +    status = GetCodecOutputData(mVideoTrack, bufferInfo, aTimeThreshold, timeout);
> +    if (status == OK || status == ERROR_END_OF_STREAM) {
> +      if (bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> +        VideoQueue().Finish();
> +      }

Do we need to handle the case that |status == ERROR_END_OF_STREAM| is true but |bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS| is zero?

@@ +597,5 @@
> +    if (v && mVideoTrack.mDiscontinuity) {
> +      v->mDiscontinuity = true;
> +      mVideoTrack.mDiscontinuity = false;
> +    }
> +    GetCallback()->OnVideoDecoded(v);

Is RequestSampleCallback::OnVideoDecoded() needed to be called even VideoQueue().PopFront() returns nullptr?

@@ +635,3 @@
>      MediaBuffer *source_buffer = nullptr;
>      MediaSource::ReadOptions options;
> +    int64_t timestamp = sInvalidTimestampUs;

nit: the intention of this change?

@@ +849,5 @@
>  }
>  
> +void
> +MediaCodecReader::ShutDownTaskQueues()
> +{

It would be better to null out the pointer to release memory resources like the other sub-functions do in MediaCodecReader::ReleaseResources().

@@ +860,5 @@
> +}
> +
> +bool
> +MediaCodecReader::CreateTaskQueues()
> +{

Since MediaCodecReader::ReallocateResources() can be triggered not only once, it would be better to check the pointer  before allocating new resources like the other sub-functions do in MediaCodecReader::ReallocateResources().

@@ +1280,5 @@
>  
> +    status = aTrack.mCodec->dequeueOutputBuffer(
> +        &info.mIndex, &info.mOffset, &info.mSize, &info.mTimeUs, &info.mFlags, duration);
> +    // Check EOS first.
> +    if (info.mFlags & MediaCodec::BUFFER_FLAG_EOS) {

Do we need to check |status == OK| as well while checking |info.mFlags & MediaCodec::BUFFER_FLAG_EOS| condition? How about the case when |status == ERROR_END_OF_STREAM| is true but |info.mFlags & MediaCodec::BUFFER_FLAG_EOS| is zero?

::: content/media/omx/MediaCodecReader.h
@@ +239,5 @@
> +  bool DecodeVideoFrameSync(int64_t aTimeThreshold);
> +  bool DecodeAudioDataTask();
> +  bool DecodeAudioDataSync();
> +  void DispatchVideoTask(int64_t aTimeThreshold);
> +  void DispatchAudioTask();

nit: It would be better to have more specific wordings on these two functions (DispatchVideoTask, DispatchAudioTask) to express the purpose of dispatched tasks (ex. the action of this task) if these two functions are not designed for all kinds of tasks (even though there is only one kind of task currently).

@@ +241,5 @@
> +  bool DecodeAudioDataSync();
> +  void DispatchVideoTask(int64_t aTimeThreshold);
> +  void DispatchAudioTask();
> +  void EnableSelfDispatch(Track& aTrack);
> +  void DisableSelfDispatch(Track& aTrack);

It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are used to prevent dispatching tasks to MediaTaskQueue during MediaTaskQueue::Flush() is called. I am not sure whether these are mandatory efforts on every async MediaDecoderReader subclass. Do we have chances to handle these efforts inside some fundamental classes?

For example, how about setting a member flag of MediaTaskQueue while entering MediaTaskQueue::Flush() and resetting the flag while leaving MediaTaskQueue::Flush(), and avoiding calling mTasks.push() when this member flag is set?
 - http://dxr.mozilla.org/mozilla-central/source/content/media/MediaTaskQueue.cpp#117
 - http://dxr.mozilla.org/mozilla-central/source/content/media/MediaTaskQueue.cpp#36

cpearce: does the above example make sense?

Comment 19

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #18)
> > +  void EnableSelfDispatch(Track& aTrack);
> > +  void DisableSelfDispatch(Track& aTrack);
> 
> It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are
> used to prevent dispatching tasks to MediaTaskQueue during
> MediaTaskQueue::Flush() is called. I am not sure whether these are mandatory
> efforts on every async MediaDecoderReader subclass. Do we have chances to
> handle these efforts inside some fundamental classes?
> 
> For example, how about setting a member flag of MediaTaskQueue while
> entering MediaTaskQueue::Flush() and resetting the flag while leaving
> MediaTaskQueue::Flush(), and avoiding calling mTasks.push() when this member
> flag is set?
>  -
> http://dxr.mozilla.org/mozilla-central/source/content/media/MediaTaskQueue.
> cpp#117
>  -
> http://dxr.mozilla.org/mozilla-central/source/content/media/MediaTaskQueue.
> cpp#36
> 
> cpearce: does the above example make sense?

add ni? flag

Comment 20

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

(In reply to Bruce Sun [:brsun] from comment #18)
> @@ +597,5 @@
> > +    if (v && mVideoTrack.mDiscontinuity) {
> > +      v->mDiscontinuity = true;
> > +      mVideoTrack.mDiscontinuity = false;
> > +    }
> > +    GetCallback()->OnVideoDecoded(v);
> 
> Is RequestSampleCallback::OnVideoDecoded() needed to be called even
> VideoQueue().PopFront() returns nullptr?

It's a bad idea to call RequestSampleCallback::OnVideoDecoded() with nullptr. Please don't!

> @@ +241,5 @@
> > +  bool DecodeAudioDataSync();
> > +  void DispatchVideoTask(int64_t aTimeThreshold);
> > +  void DispatchAudioTask();
> > +  void EnableSelfDispatch(Track& aTrack);
> > +  void DisableSelfDispatch(Track& aTrack);
> 
> It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are
> used to prevent dispatching tasks to MediaTaskQueue during
> MediaTaskQueue::Flush() is called.

Hmm... Thanks for bringing this up Bruce. It's not clear why we need Enable/DisableSelfDispatch. All the calls to Enable/DisableSelfDispatch() are on the decode task queue, and so are the calls to the Track::mTaskQueue objects. So it should be impossible to interleave dispatching tasks to the Track::mTaskQueues and flushing those task queues.

bchen: can you remove the Enable/DisableSelfDispatch calls? Does anything break if you do? I don't think it would...

Comment 21

[Benjamin Chen [:bechen]]

(In reply to Chris Pearce (:cpearce) from comment #20)
> (In reply to Bruce Sun [:brsun] from comment #18)
> > > +  void EnableSelfDispatch(Track& aTrack);
> > > +  void DisableSelfDispatch(Track& aTrack);
> > 
> > It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are
> > used to prevent dispatching tasks to MediaTaskQueue during
> > MediaTaskQueue::Flush() is called.
> 
> Hmm... Thanks for bringing this up Bruce. It's not clear why we need
> Enable/DisableSelfDispatch. All the calls to Enable/DisableSelfDispatch()
> are on the decode task queue, and so are the calls to the Track::mTaskQueue
> objects. So it should be impossible to interleave dispatching tasks to the
> Track::mTaskQueues and flushing those task queues.
> 
> bchen: can you remove the Enable/DisableSelfDispatch calls? Does anything
> break if you do? I don't think it would...

Remove Enable/DisableSelfDispatch will break the code flow because the DecodeAudioDataSync/DecodeVideoFrameSync will dispatch new task to itself if GetCodecOutputData returns -EAGAIN.

As Bruce says, Enable/DisableSelfDispatch just stop dispatching task to the task queue. Maybe we can write a subclass |MediaCodecReaderTaskQueue :pubic MediaTaskQueue| and remove the MOZ_FINAL in MediaTaskQueue.h.

Comment 22

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

(In reply to Benjamin Chen [:bechen] from comment #21)
> (In reply to Chris Pearce (:cpearce) from comment #20)
> > (In reply to Bruce Sun [:brsun] from comment #18)
> > > > +  void EnableSelfDispatch(Track& aTrack);
> > > > +  void DisableSelfDispatch(Track& aTrack);
> > > 
> > > It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are
> > > used to prevent dispatching tasks to MediaTaskQueue during
> > > MediaTaskQueue::Flush() is called.
> > 
> > Hmm... Thanks for bringing this up Bruce. It's not clear why we need
> > Enable/DisableSelfDispatch. All the calls to Enable/DisableSelfDispatch()
> > are on the decode task queue, and so are the calls to the Track::mTaskQueue
> > objects. So it should be impossible to interleave dispatching tasks to the
> > Track::mTaskQueues and flushing those task queues.
> > 
> > bchen: can you remove the Enable/DisableSelfDispatch calls? Does anything
> > break if you do? I don't think it would...
> 
> Remove Enable/DisableSelfDispatch will break the code flow because the
> DecodeAudioDataSync/DecodeVideoFrameSync will dispatch new task to itself if
> GetCodecOutputData returns -EAGAIN.

Ah, DecodeAudioDataSync runs on the mAudioTrack::mTaskQueue, and you call Flush() on the decode task queue.


> As Bruce says, Enable/DisableSelfDispatch just stop dispatching task to the
> task queue. Maybe we can write a subclass |MediaCodecReaderTaskQueue :pubic
> MediaTaskQueue| and remove the MOZ_FINAL in MediaTaskQueue.h.

MediaTaskQueue is MOZ_FINAL in order to prevent you from inheriting from it!

I think you should instead change MediaTaskQueue::Flush() to set a flag mIsFlushing=true, and then MediaTaskQueue::Dispatch() should check that flag and return NS_ERROR_ABORT if called when mIsFlushing is true.

Note that MediaTaskQueue::Flush() will not return until the task running in it has finished.

Does that make sense? If you change MediaTaskQueue I want to review this patch again.

Comment 23

[Benjamin Chen [:bechen]]

(In reply to Bruce Sun [:brsun] from comment #18)
> Comment on attachment 8470749 [details] [diff] [review]
> bug-1033912.v03.patch
> 
> Review of attachment 8470749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great. Just want to double-check for some further information.
> 
> ::: content/media/omx/MediaCodecReader.cpp
> @@ +298,5 @@
> >      status = GetCodecOutputData(mAudioTrack, bufferInfo, sInvalidTimestampUs, timeout);
> >      if (status == OK || status == ERROR_END_OF_STREAM) {
> > +      if (bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> > +        AudioQueue().Finish();
> > +      }
> 
> Do we need to handle the case that |status == ERROR_END_OF_STREAM| is true
> but |bufferInfo.mFlags & MediaCodec::BUFFER_FLAG_EOS| is zero?
> 

By the way there is a flaw here, if an audio/video sample carries the EOS flag, the code flow will call 
AudioQueue().Finish() first then put the sample into the queue.

> 
> @@ +1280,5 @@
> >  
> > +    status = aTrack.mCodec->dequeueOutputBuffer(
> > +        &info.mIndex, &info.mOffset, &info.mSize, &info.mTimeUs, &info.mFlags, duration);
> > +    // Check EOS first.
> > +    if (info.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> 
> Do we need to check |status == OK| as well while checking |info.mFlags &
> MediaCodec::BUFFER_FLAG_EOS| condition? How about the case when |status ==
> ERROR_END_OF_STREAM| is true but |info.mFlags & MediaCodec::BUFFER_FLAG_EOS|
> is zero?
> 

Yeah, seems we need to handle this case here in |GetCodecOutputData|
For now, it looks like the MediaCodec::BUFFER_FLAG_EOS only output once but the return value ERROR_END_OF_STREAM might be returned many times if the stream reaches the end.

Comment 24

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

(In reply to Chris Pearce (:cpearce) from comment #22)
> (In reply to Benjamin Chen [:bechen] from comment #21)
> > (In reply to Chris Pearce (:cpearce) from comment #20)
> > > (In reply to Bruce Sun [:brsun] from comment #18)
> > > > > +  void EnableSelfDispatch(Track& aTrack);
> > > > > +  void DisableSelfDispatch(Track& aTrack);
> > > > 
> > > > It seems these two functions (EnableSelfDispatch, DisableSelfDispatch) are
> > > > used to prevent dispatching tasks to MediaTaskQueue during
> > > > MediaTaskQueue::Flush() is called.
> > > 
> > > Hmm... Thanks for bringing this up Bruce. It's not clear why we need
> > > Enable/DisableSelfDispatch. All the calls to Enable/DisableSelfDispatch()
> > > are on the decode task queue, and so are the calls to the Track::mTaskQueue
> > > objects. So it should be impossible to interleave dispatching tasks to the
> > > Track::mTaskQueues and flushing those task queues.
> > > 
> > > bchen: can you remove the Enable/DisableSelfDispatch calls? Does anything
> > > break if you do? I don't think it would...
> > 
> > Remove Enable/DisableSelfDispatch will break the code flow because the
> > DecodeAudioDataSync/DecodeVideoFrameSync will dispatch new task to itself if
> > GetCodecOutputData returns -EAGAIN.
> 
> Ah, DecodeAudioDataSync runs on the mAudioTrack::mTaskQueue, and you call
> Flush() on the decode task queue.
> 
> 
> > As Bruce says, Enable/DisableSelfDispatch just stop dispatching task to the
> > task queue. Maybe we can write a subclass |MediaCodecReaderTaskQueue :pubic
> > MediaTaskQueue| and remove the MOZ_FINAL in MediaTaskQueue.h.
> 
> MediaTaskQueue is MOZ_FINAL in order to prevent you from inheriting from it!
> 
> I think you should instead change MediaTaskQueue::Flush() to set a flag
> mIsFlushing=true, and then MediaTaskQueue::Dispatch() should check that flag
> and return NS_ERROR_ABORT if called when mIsFlushing is true.

The WIP patch I just uploaded to bug 1053008 does this.

Comment 25

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v04.patch

1. remove Enable/DisableSelfDispatch functions based on bug 1053008
2. address comment 18.

Comment 26

[Benjamin Chen [:bechen]]

Attachment: bug-1033912-part2-nits.patch

r=cpearce

Comment 27

[Bruce Sun [:brsun]]

Comment on attachment 8473604 [details] [diff] [review]
bug-1033912.v04.patch

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

::: content/media/omx/MediaCodecReader.cpp
@@ +224,5 @@
>    ReleaseResources();
>  }
>  
>  bool
> +MediaCodecReader::CheckVideoResources()

It would be better to inline this function.

@@ +232,3 @@
>  
> +bool
> +MediaCodecReader::CheckAudioResources()

It would be better to inline this function.

@@ +238,5 @@
> +
> +void
> +MediaCodecReader::DispatchAudioTask()
> +{
> +  if (mAudioTrack.mTaskQueue->IsEmpty()) {

|mAudioTrack.mTaskQueue| pointer should be checked before being dereferenced within this function body.

@@ +249,5 @@
> +
> +void
> +MediaCodecReader::DispatchVideoTask(int64_t aTimeThreshold)
> +{
> +  if (mVideoTrack.mTaskQueue->IsEmpty()) {

|mVideoTrack.mTaskQueue| pointer should be checked before being dereferenced within this function body.

@@ +1279,5 @@
> +    if (info.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> +      aBuffer = info;
> +      aBuffer.mBuffer = aTrack.mOutputBuffers[info.mIndex];
> +      aTrack.mOutputEndOfStream = true;
> +      return ERROR_END_OF_STREAM;

Should |MediaCodecReader::GetCodecOutputData()| return ERROR_END_OF_STREAM as well when |MediaCodec::dequeueOutputBuffer()| returns ERROR_END_OF_STREAM?

Comment 28

[Benjamin Chen [:bechen]]

Attachment: bug-1033912-part2-nits.patch

Comment 29

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v04.patch

try server:
https://tbpl.mozilla.org/?tree=Try&rev=9ff035bdb5eb

Comment 30

[Benjamin Chen [:bechen]]

Attachment: bug-1033912.v04.patch

r=cpearce, r=brsun
The try server result looks good.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aeaa1024b2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/faef3e2c3420

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0aeaa1024b2b
https://hg.mozilla.org/mozilla-central/rev/faef3e2c3420