Closed
Bug 1101374
Opened 10 years ago
Closed 9 years ago
MediaCodec generates invalid PTS MediaBuffers on some videos
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: ayang, Assigned: ayang)
References
Details
Attachments
(2 files, 2 obsolete files)
63.75 KB,
text/xml
|
Details | |
12.04 KB,
patch
|
ayang
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8526519 -
Flags: feedback?(bwu)
Comment 2•10 years ago
|
||
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.
Attachment #8526519 -
Flags: feedback?(bwu)
Updated•10 years ago
|
Attachment #8526519 -
Flags: feedback+
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8526519 -
Attachment is obsolete: true
Attachment #8526581 -
Flags: review?(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|?
Attachment #8526581 -
Flags: review?(edwin)
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
(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 on attachment 8526581 [details] [diff] [review] add_time_info_for_decoded_frame Alright. r+ with nits fixed.
Attachment #8526581 -
Flags: review+
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
No testcase for Gonk PDM, it is safe to check it without try.
Attachment #8526581 -
Attachment is obsolete: true
Attachment #8528275 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b6785b8cb812
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•