In bug 1294552, we're getting errors because frames are tagged as keyframes when they are not. This is okay if you play the stream from 0. But when seeking, this will cause decoding errors. We've also often seen cases where mp4 container have all their frames tagged as keyframes causing playback error when seeking. For non-encrypted content, we could simply scan the h264 data header and determine there the frame type (I, P or B frame)... It should be fairly trivial and would be much more reliable than trusting the mp4 container. We've already stopped trusting the container for determining the video dimensions and aspect ratio. Not trusting the keyframe marking isn't a stretch.

Additionally, it would make much easier the reporting of content incorrectly marked as keyframe, and notify the JS/Console

Comment on attachment 8787966 [details] Bug 1300296: P1. Add method to determine if an H264 frame is an I-Frame. https://reviewboard.mozilla.org/r/76524/#review74710 ::: media/libstagefright/binding/H264.cpp:515 (Diff revision 2) > + case 4: nalLen = reader.ReadU32(); break; > + } > + if (!nalLen) { > + continue; > + } > + const uint8_t* p = reader.Read(nalLen); I presume the ByteReader bounds-checks

Comment on attachment 8787966 [details] Bug 1300296: P1. Add method to determine if an H264 frame is an I-Frame. https://reviewboard.mozilla.org/r/76524/#review74712 ::: media/libstagefright/binding/H264.cpp:515 (Diff revision 2) > + case 4: nalLen = reader.ReadU32(); break; > + } > + if (!nalLen) { > + continue; > + } > + const uint8_t* p = reader.Read(nalLen); Yes... ByteReader prevents ever reading outside of the provided boundaries. ByteReader::Reader will return null if there's not enough data available.

Comment on attachment 8787966 [details] Bug 1300296: P1. Add method to determine if an H264 frame is an I-Frame. https://reviewboard.mozilla.org/r/76524/#review74904 ::: media/libstagefright/binding/H264.cpp:500 (Diff revision 2) > + // We must have a valid AVCC frame with extradata. > + return FrameType::INVALID; > + } > + MOZ_ASSERT(aSample->Data()); > + > + int nalLenSize = ((*aSample->mExtraData)[4] & 3) + 1; Do you need to check that the length of mExtraData is >= 5?

Comment on attachment 8787967 [details] Bug 1300296: P2. Don't rely on MP4 container to properly report if a frame is a keyframe. https://reviewboard.mozilla.org/r/76526/#review74906 With this patch we take the sample and modify it in two differnet ways. It would be easy to make a mistake about which fields have been updated. It would be much simpler to replace mIterator->GetNext() universally with GetNextSampleInternal() that looks something like this: MediaRawSample* GetNextSampleInternal() { MediaRawSample* sample = mIterator->GetNext(); if (!sample) { return nullptr; } if (IsIncorrectlyMarkedAsKeyframe(sample)) { NS_WARNING(nsPrintfCString("Frame incorrectly marked as keyframe @ pts:%lld dur:%u dts:%lld", aSample->mTime, aSample->mDuration, aSample->mTimecode).get()); aSample->mKeyframe = false; } if (sample->mCrypto.mValid) { nsAutoPtr<MediaRawDataWriter> writer(sample->CreateWriter()); writer->mCrypto.mMode = mInfo->mCrypto.mMode; writer->mCrypto.mIVSize = mInfo->mCrypto.mIVSize; writer->mCrypto.mKeyId.AppendElements(mInfo->mCrypto.mKeyId); } return sample; } We can get rid of the SPS telemetry. The SetNextKeyFrameTime() logic would have to live somewhere else. It doesn't really make sense where it is anyway. ::: dom/media/fmp4/MP4Demuxer.cpp:347 (Diff revision 2) > } > > if (samples->mSamples.IsEmpty()) { > return SamplesPromise::CreateAndReject(DemuxerFailureReason::END_OF_STREAM, __func__); > } else { > UpdateSamples(samples->mSamples); UpdateSamples and CheckSample do the same thing. You could remove the loop in UpdateSamples by calling it in the same place as CheckSample. ::: dom/media/fmp4/MP4Demuxer.cpp:374 (Diff revision 2) > SetNextKeyFrameTime(); > } > > void > +MP4TrackDemuxer::CheckSample(MediaRawData* aSample) > +{ CheckSample is poorly named because it modifies aSample. ::: dom/media/fmp4/MP4Demuxer.cpp:378 (Diff revision 2) > +MP4TrackDemuxer::CheckSample(MediaRawData* aSample) > +{ > + if (mInfo->GetAsVideoInfo()) { > + aSample->mExtraData = mInfo->GetAsVideoInfo()->mExtraData; > + if (mIsH264) { > + mp4_demuxer::H264::FrameType type = mp4_demuxer::H264::GetFrameType(aSample); You could throw away most of the function and write: if (mIsH264 && aSample->mKeyframe && mp4_demuxer::H264::GetFrameType(aSample) != mp4_demuxer::H264::FrameType::I_FRAME) { NS_WARNING(nsPrintfCString("Frame incorrectly marked as keyframe @ pts:%lld dur:%u dts:%lld", aSample->mTime, aSample->mDuration, aSample->mTimecode).get()); aSample->mKeyframe = false; }

Comment on attachment 8787967 [details] Bug 1300296: P2. Don't rely on MP4 container to properly report if a frame is a keyframe. https://reviewboard.mozilla.org/r/76526/#review74906 I'll take the SPS related stuff in a separate bug... will leave stuff as is for now

Comment on attachment 8787967 [details] Bug 1300296: P2. Don't rely on MP4 container to properly report if a frame is a keyframe. https://reviewboard.mozilla.org/r/76526/#review74906 > You could throw away most of the function and write: > > if (mIsH264 > && aSample->mKeyframe > && mp4_demuxer::H264::GetFrameType(aSample) != mp4_demuxer::H264::FrameType::I_FRAME) { > NS_WARNING(nsPrintfCString("Frame incorrectly marked as keyframe @ pts:%lld dur:%u dts:%lld", > aSample->mTime, aSample->mDuration, aSample->mTimecode).get()); > aSample->mKeyframe = false; > } no.. I want to be able to distinguish three cases, not just "not keyframe" I'll handle the issue where a sample is actually invalid at a later stage.

Comment on attachment 8787966 [details] Bug 1300296: P1. Add method to determine if an H264 frame is an I-Frame. https://reviewboard.mozilla.org/r/76524/#review74904 > Do you need to check that the length of mExtraData is >= 5? yes, because it's the minimum size for a proper AVCC (either AVC1 or AVC3) extradata. With AVCC, the NAL is preceded by its size, the size of a NAL is encoded either as 1,2, 3 or 4 bytes. The size of the NAL length is stored in byte 4 of the extradata. Hence checking that size is >= 5.

Comment on attachment 8787966 [details] Bug 1300296: P1. Add method to determine if an H264 frame is an I-Frame. https://reviewboard.mozilla.org/r/76524/#review74904 > yes, because it's the minimum size for a proper AVCC (either AVC1 or AVC3) extradata. With AVCC, the NAL is preceded by its size, the size of a NAL is encoded either as 1,2, 3 or 4 bytes. The size of the NAL length is stored in byte 4 of the extradata. Hence checking that size is >= 5. forgot to add that the check that mExtraData size is >= 5 is done in the AnnexB::IsAVCC which is return aSample->Size() >= 3 && aSample->mExtraData && aSample->mExtraData->Length() >= 7 && (*aSample->mExtraData)[0] == 1;

Comment on attachment 8788369 [details] Bug 1300296: Use valid and spec-compliant MP4 file for all tests. https://reviewboard.mozilla.org/r/76874/#review75004 I'm mostly trusting you about the changes to the file and corresponding updates to the test data since I'm not an expert on that. Otherwise this looks good. ::: testing/web-platform/tests/media-source/mediasource-sequencemode-append-buffer.html:63 (Diff revision 1) > "expectedTimestampOffset"); > > // Prior to EOS, the buffered range end time may not have fully reached the next media > // segment's timecode (adjusted by any timestampOffset). It should not exceed it though. > // Therefore, an exact assertBufferedEquals() will not work here. > - assert_equals(sourceBuffer.buffered.length, 1, "sourceBuffer.buffered has 1 range before EOS"); > + assert_greater_than(sourceBuffer.buffered.length, 0, "sourceBuffer.buffered has more than 1 range before EOS"); "more than 1" seems inaccurate since 1 is allowed by the condition.

Comment on attachment 8788368 [details] Bug 1300296: P3. Ensure that a new or flushed decoder first h264 frame is always a keyframe. https://reviewboard.mozilla.org/r/76872/#review75012 r+ with suggestion: ::: dom/media/platforms/wrappers/H264Converter.cpp:62 (Diff revision 1) > + if (mNeedKeyframe && !aSample->mKeyframe) { > + return NS_OK; > + } > + mNeedKeyframe = false; Tiny optimization possible: if (mNeedKf) { if (!aS->mKf) { return NS_OK; } mNeedKf = false; } This way you won't overwrite mNeedKf when it's already false.

Comment on attachment 8788369 [details] Bug 1300296: Use valid and spec-compliant MP4 file for all tests. https://reviewboard.mozilla.org/r/76874/#review75014 r+ with nits in commit description: ::: testing/web-platform/meta/media-source/mediasource-sequencemode-append-buffer.html.ini:1 (Diff revision 1) > +[mediasource-sequencemode-append-buffer.html] In commit description: 'such as' -> 'such that' 'remain' -> 'remains' 'as the previous files' -> 'as in the previous file' 'Total duration' -> 'The total duration' 'Like the' -> 'Like in the'

Comment on attachment 8787967 [details] Bug 1300296: P2. Don't rely on MP4 container to properly report if a frame is a keyframe. https://reviewboard.mozilla.org/r/76526/#review75368

P3 causes failure of the video decoding suspend test due to the way the blank decoder is substituted from the original decoder half-way. If this happens in the middle of a GOP, then all frames seen are now dropped because we don't have a starting keyframes... The blank decoder should never be wrapped in a H264Converter

Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/d77a82f4a665 P1. Add method to determine if an H264 frame is an I-Frame. r=jesup https://hg.mozilla.org/mozilla-central/rev/06e25a05496f P2. Don't rely on MP4 container to properly report if a frame is a keyframe. r=kentuckyfriedtakahe https://hg.mozilla.org/mozilla-central/rev/d7e38eda81db P3. Ensure that a new or flushed decoder first h264 frame is always a keyframe. r=gerald https://hg.mozilla.org/mozilla-central/rev/ba7d7fe0ea3c Use valid and spec-compliant MP4 file for all tests. r=gerald,jgraham

https://hg.mozilla.org/mozilla-central/rev/d77a82f4a665 https://hg.mozilla.org/mozilla-central/rev/06e25a05496f https://hg.mozilla.org/mozilla-central/rev/d7e38eda81db https://hg.mozilla.org/mozilla-central/rev/ba7d7fe0ea3c