Closed Bug 1300296 Opened 4 years ago Closed 4 years ago

Do not rely on container information to determine if a h264 frame is a keyframe

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

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
Attachment #8787966 - Flags: review?(rjesup) → review+
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;
}
Attachment #8787967 - Flags: review?(ajones) → review-
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.
Attachment #8788369 - Flags: review?(james) → review+
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.
Attachment #8788368 - Flags: review?(gsquelart) → review+
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'
Attachment #8788369 - Flags: review?(gsquelart) → review+
Assignee: nobody → jyavenard
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
Attachment #8787967 - Flags: review?(ajones) → review+
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
See Also: → 1301059
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
Blocks: 1296211
Depends on: 1359058
You need to log in before you can comment on or make changes to this bug.