1101374 - MediaCodec generates invalid PTS MediaBuffers on some videos

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

http://people.mozilla.org/~ayang/mp4/aspect_mp4.html

I/PRLog   ( 1743): 2014-11-18 11:15:51.551546 UTC - 4943328[b1061e80]: Decoded Video sample time=9920000 dur=1
D/MediaCodecProxy( 1743): Output returned -11
I/PRLog   ( 1743): 2014-11-18 11:15:51.577520 UTC - 4938656[b0fc8380]: Decoder=b092ef00 discarding video frame mTime=9840000 clock_time=9840234 (1 so far)
I/PRLog   ( 1743): 2014-11-18 11:15:51.577959 UTC - 4938656[b0fc8380]: Decoder=b092ef00 playing video frame 9840001
V/SampleIterator( 1743): seekTo(251)
I/PRLog   ( 1743): 2014-11-18 11:15:51.578727 UTC - 4939240[b0fc9080]: PopSample Video time=10040000 dur=1
D/GonkVideoDecoderManager( 1743): Input mp4 sample, composition time 10040000
D/GonkVideoDecoderManager( 1743): VideoData PTS: 9920001, KeyFrame: 0
I/PRLog   ( 1743): 2014-11-18 11:15:51.594002 UTC - 4943328[b1061e80]: Decoded Video sample time=9920001 dur=1
D/GonkVideoDecoderManager( 1743): VideoData PTS: 80000, KeyFrame: 0
I/PRLog   ( 1743): 2014-11-18 11:15:51.595538 UTC - 4943328[b1061e80]: Decoded Video sample time=80000 dur=1
D/GonkVideoDecoderManager( 1743): VideoData PTS: 1, KeyFrame: 0
I/PRLog   ( 1743): 2014-11-18 11:15:51.597952 UTC - 4943328[b1061e80]: Decoded Video sample time=1 dur=1
D/GonkVideoDecoderManager( 1743): VideoData PTS: 0, KeyFrame: 0
I/PRLog   ( 1743): 2014-11-18 11:15:51.599201 UTC - 4943328[b1061e80]: Decoded Video sample time=0 dur=1


Decoded video sample timestamp rollbacks to 80000, 1, 0 and it causes MediaDecoderStateMachine skips to next key frame.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: add_time_info_for_decoded_frame

Comment 2

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

Comment on attachment 8526519 [details] [diff] [review]
add_time_info_for_decoded_frame

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

Overall looks good to me!
Just some suggestions.

::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +134,5 @@
>    // it's executing at all. Note the MP4Reader ignores all output while
>    // flushing.
>    mTaskQueue->Flush();
>  
>    status_t err = mDecoder->flush();

Could you also help put mDecoder->flush() into mManager->Flush?
mDecoder is created in mManager. We should call it from mManager instead of there. The original design is required to be changed.

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +178,5 @@
> +      mFrameTimeInfo.RemoveElementsAt(0, i+1);
> +      break;
> +    }
> +  }
> +

Perhaps it would be better to put this codes to a function and call it here.

@@ +446,5 @@
>  
> +nsresult
> +GonkVideoDecoderManager::Flush()
> +{
> +  mFrameTimeInfo.Clear();

We may need a lock to protect mFrameTimeInfo not being cleared when being checked in CreateVideoData().
Or we may dispatch a task for flush to mTaskQueue in GonkMediaDataDecoder.

Comment 3

[Alfredo Yang (:alfredo)]

(In reply to Blake Wu [:bwu][:blakewu] from comment #2)
> Comment on attachment 8526519 [details] [diff] [review]
> add_time_info_for_decoded_frame
> 
> Review of attachment 8526519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good to me!
> Just some suggestions.
> 
> ::: dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp
> @@ +134,5 @@
> >    // it's executing at all. Note the MP4Reader ignores all output while
> >    // flushing.
> >    mTaskQueue->Flush();
> >  
> >    status_t err = mDecoder->flush();
> 
> Could you also help put mDecoder->flush() into mManager->Flush?
> mDecoder is created in mManager. We should call it from mManager instead of
> there. The original design is required to be changed.
> 
> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ +178,5 @@
> > +      mFrameTimeInfo.RemoveElementsAt(0, i+1);
> > +      break;
> > +    }
> > +  }
> > +
> 
> Perhaps it would be better to put this codes to a function and call it here.
> 
> @@ +446,5 @@
> >  
> > +nsresult
> > +GonkVideoDecoderManager::Flush()
> > +{
> > +  mFrameTimeInfo.Clear();
> 
> We may need a lock to protect mFrameTimeInfo not being cleared when being
> checked in CreateVideoData().
> Or we may dispatch a task for flush to mTaskQueue in GonkMediaDataDecoder.

Thanks for comments. I'll update patch.

Comment 4

[Alfredo Yang (:alfredo)]

Attachment: add_time_info_for_decoded_frame

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8526581 [details] [diff] [review]
add_time_info_for_decoded_frame

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

Is the (pts,duration) table necessary? Where is the pts getting corrupted?

More questions inline.

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +141,5 @@
> +  // these frames are old frame before seeking.
> +  aDuration = 1;
> +  for (uint32_t i = 0; i < mFrameTimeInfo.Length(); i++) {
> +    FrameTimeInfo entry;
> +    memcpy(&entry, &mFrameTimeInfo.ElementAt(i), sizeof(FrameTimeInfo));

const FrameTimeInfo& entry = mFrameTimeInfo.ElementAt(i);

@@ +145,5 @@
> +    memcpy(&entry, &mFrameTimeInfo.ElementAt(i), sizeof(FrameTimeInfo));
> +    if (i == 0) {
> +      if (entry.pts > aPTS) {
> +        // Codec sent a frame with rollbacked PTS time. It could
> +        // be codec's problem.

Is it possible that this path could be hit after seeking backwards?

@@ +152,5 @@
> +      }
> +    }
> +
> +    // Ideally, the first entry in mFrameTimeInfo should be the one we are looking
> +    // for. However, MediaCodec could dropped frame and the first entry dones'

s/dones'/doesn't/

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.h
@@ +105,5 @@
>  
> +  // FrameTimeInfo keeps the presentation time stamp (pts) and its duration.
> +  // On MediaDecoderStateMachine, it needs pts and duration to display decoded
> +  // frame correctly. But OMX can carry one field of time info (kKeyTime) so
> +  // we use FrameTimeInfo to keep pts and duration.

Why can't we pass duration with |kKeyDuration|?

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: pixel_aspect_ratio_info.xml

Comment 7

[Alfredo Yang (:alfredo)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #5)
> Comment on attachment 8526581 [details] [diff] [review]
> add_time_info_for_decoded_frame
> 
> Review of attachment 8526581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the (pts,duration) table necessary? Where is the pts getting corrupted?

It's my bad that doesn't explain it well.
On comment 0:
"Decoded Video sample time=9920001 dur=1", the PTS of decoded frame is about 10s (9920001 sample, timescale = 1000000)
However the next 3 decoded video frames from OMX are:
"Decoded Video sample time=80000",
"Decoded Video sample time=1" and
"Decoded Video sample time=0".
These corrupted PTS cause MediaDecoderStateMachine jumping into skipToNextKeyFrame.

The first video frame of this video is not a valid I frame and according to the ISO dump in pixel_aspect_ratio_info.xml.
I think it should be OMX codec problem to keep these invalid frames for a while and output it unexpectedly.

MediaOmxReader has the same problem but somehow skipToNextKeyFrame is not working. So this video plays fine in MediaOmxReader.


"duration" is not necessary, set duration to 1 seems ok. But MediaDecoderStateMachine uses it for A/V sync, so I add a "duration, pts" table.

> @@ +145,5 @@
> > +    memcpy(&entry, &mFrameTimeInfo.ElementAt(i), sizeof(FrameTimeInfo));
> > +    if (i == 0) {
> > +      if (entry.pts > aPTS) {
> > +        // Codec sent a frame with rollbacked PTS time. It could
> > +        // be codec's problem.
> 
> Is it possible that this path could be hit after seeking backwards?

Yes, it is possible.
I think it should be ok to discard these frames because these are frames before seeking. The new seeking frame should be displayed as quick as possible.

> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.h
> @@ +105,5 @@
> >  
> > +  // FrameTimeInfo keeps the presentation time stamp (pts) and its duration.
> > +  // On MediaDecoderStateMachine, it needs pts and duration to display decoded
> > +  // frame correctly. But OMX can carry one field of time info (kKeyTime) so
> > +  // we use FrameTimeInfo to keep pts and duration.
> 
> Why can't we pass duration with |kKeyDuration|?

From [1], kKeyDuration is for the track duration, not a single frame.

[1] http://androidxref.com/4.4.4_r1/xref/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#964

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8526581 [details] [diff] [review]
add_time_info_for_decoded_frame

Alright. r+ with nits fixed.

Comment 9

[Alfredo Yang (:alfredo)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #8)
> Comment on attachment 8526581 [details] [diff] [review]
> add_time_info_for_decoded_frame
> 
> Alright. r+ with nits fixed.

Thanks for review.

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: add_time_info_for_decoded_frame

No testcase for Gonk PDM, it is safe to check it without try.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6785b8cb812

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6785b8cb812