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

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Additionally, it would make much easier the reporting of content incorrectly marked as keyframe, and notify the JS/Console
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

3 years ago
mozreview-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/#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+
(Assignee)

Comment 7

3 years ago
mozreview-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 8

3 years ago
mozreview-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/#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-
(Assignee)

Comment 10

3 years ago
mozreview-review-reply
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
(Assignee)

Comment 11

3 years ago
mozreview-review-reply
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.
(Assignee)

Comment 12

3 years ago
mozreview-review-reply
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.
(Assignee)

Comment 13

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

3 years ago
mozreview-review
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 18

3 years ago
mozreview-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 19

3 years ago
mozreview-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)

Updated

3 years ago
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+
(Assignee)

Comment 21

3 years ago
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
(Assignee)

Updated

3 years ago
See Also: → bug 1301059
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 26

3 years ago
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

Comment 27

3 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1296211
(Assignee)

Updated

2 years ago
Depends on: 1359058
You need to log in before you can comment on or make changes to this bug.