Open Bug 1670333 Opened 2 years ago Updated 3 months ago

When decoding video by OpenH264 GMP and position at video stream is changed, decoding fails. (Mozilla could take Fedora's downstream fix: https://src.fedoraproject.org/rpms/firefox/blob/rawhide/f/mozilla-1670333.patch )

Categories

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

Desktop
Linux
defect

Tracking

()

People

(Reporter: stransky, Unassigned)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file)

When user seeks in video during playback repeatedly, playback fails with "Media error". Happens also without a patch from Bug 1663844.

The failure comes from GMPVideoDecoderParent::Decode() where this condition fails:

if ((NumInUse(GMPSharedMem::kGMPFrameData) > 3 * GMPSharedMem::kGMPBufLimit) || (NumInUse(GMPSharedMem::kGMPEncodedData) > GMPSharedMem::kGMPBufLimit)) {

GMPSharedMem::kGMPBufLimit is 20 and NumInUse(GMPSharedMem::kGMPEncodedData is 21.

When the GMPSharedMem::kGMPBufLimit limit is raised to 40 the limit scenario won't happen but the openh264 decoder (gmp-openh264 plugin) fails at DecodeFrameNoDelay() with decoding error 34 (still investigating).

The error is:

dsRefLost = 0x02, ///< layer lost at reference frame with temporal id 0
dsDataErrorConcealed = 0x20, ///< current data error concealed specified

Bryce, would you mind taking a look?

Severity: -- → S3
Flags: needinfo?(bvandyk)

The decoding error is related to on-line playback (I use yotube). When I open the same video locally I can't reproduce it - I can seek in the video without any issue. So it looks like we serve incomplete data to the codec during on-line playback.

Could you post steps to repro so I can make sure I'm looking at the same thing? I'm assuming the cases where this happens without the patch mentioned in comment 0 are those where we have non-moz builds using OpenH264?

It sounds like we have both shared memory and decoding related issues.

Does the decoding problem only take place when changing position?

Flags: needinfo?(stransky)

I'm investigating that right now.

Looks like when video is played from YT we don't feed the decoder with correct data and/or Reset() call is not correctly implemented in gmp-openh264 plugin. The gmp-openh264 does not recognize the frames after seek as a new begin with a new start but tries to continue to decode the existing one and complains about a frame gap and throw an error.

The continuous frame stream uses NAL_UNIT_CODED_SLICE frames and the first frame (new start) uses NAL_UNIT_CODED_SLICE_IDR. After seek from disk we send NAL_UNIT_CODED_SLICE_IDR frame to decoder so it begins a new decoding but with seek on YT we send a new frame with NAL_UNIT_CODED_SLICE do the decoder tries to continue with the current stream.

Flags: needinfo?(stransky)

Bryce, how do we handle stream decode here? When we seek in stream and we get i-frames which can't be send to decoder - should we drop i-frames until we get a keyframe and start the decoding there?

(In reply to Martin Stránský [:stransky] from comment #6)

Bryce, how do we handle stream decode here? When we seek in stream and we get i-frames which can't be send to decoder - should we drop i-frames until we get a keyframe and start the decoding there?

That's what we should do. Explicitly

  • We begin a seek.
  • We flush the decoder[0] -- drop the frames we're currently holding a prepare to decode from the new seek point.
  • We seek the demuxer[1] to the desired time. The demuxer abstracts this operation and returns the actual time it has seeked to. This will normally be a random access point, which is expected to be a keyframe.
  • We start feeding the decoder with samples from that point.

(The MediaFormatReader's SeekInternal[2] is a good place to delve from if you want to look more).

Because the container can disagree with the h264 stream itself we attempt to determine the frame types by reading the stream[3].

Let me know if I can provide more information, or if it would help to have me debug this as well.

[0] https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/dom/media/platforms/PlatformDecoderModule.h#325
[1] https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/dom/media/MediaDataDemuxer.h#147
[2] https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/dom/media/MediaFormatReader.cpp#1952
[3] https://searchfox.org/mozilla-central/rev/803b368879fa332e8e2c1840bf1ec164f7ed2c32/dom/media/platforms/agnostic/bytestreams/H264.cpp#942

Flags: needinfo?(bvandyk)

Hi Bryce,

thanks a lot! There's a how-to enable openh264 with latest trunk:

  1. apply a patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1663844 to make open264 usable for media playback
  2. apply a patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1667096 and build with --with-system-fdk-aac, you need to have fdk-aac-free-devel or fdk-aac-devel packages installed. This may be optional as YT uses Opus afaik.
  3. set media.gmp.decoder.enabled to true
  4. disable ffmpeg playback by setting media.ffmpeg.enabled to false
  5. install openh264 system wide as Mozilla delivers 1.8.1 only (Fedora provides mozilla-openh264-2.1.1 package)
  6. install enhanced-h264ify extension and disable VP8/9/AV1 video formats.
  7. Run youtube, open h264 video, seek a video, you get the error.

NI me to try repro.

Flags: needinfo?(bvandyk)

I found the problem - we generally mark all mark i-frames as keyframes. FFmpeg accepts that but openh264 refuses to start decoding with i-frame.

Attached file a simple workaround

There's a simple workaround for the seek error. I wonder what's the original intention here.

The problem is perhaps that we both H264_NAL_IDR_SLICE and H264_NAL_SLICE mark as I_FRAME and consider them as keyframe. But only H264_NAL_IDR_SLICE are the keyframes which we're looking for in seek:

if (nalType == H264_NAL_IDR_SLICE) {
  // IDR NAL.
  return FrameType::I_FRAME;
} else if (nalType == H264_NAL_SEI) {

} else if (nalType == H264_NAL_SLICE) {
  RefPtr<mozilla::MediaByteBuffer> decodedNAL = DecodeNALUnit(p, nalLen);
  if (DecodeISlice(decodedNAL)) {
    return FrameType::I_FRAME;
  }
}

I'm sure Jean-Yves should know more.

Flags: needinfo?(jyavenard)

The code comes from Bug 1300296.

We've done various relaxing and tweaking of the frames we consider key frames to handle the different content we get served. Bug 1539182 + Bug 1347518 are a couple of the major ones. If OpenH264 must start decoding on an IDR explicitly then I imagine the current code is not going to play nice with it.

(In reply to Bryce Seager van Dyk (:bryce) from comment #16)

We've done various relaxing and tweaking of the frames we consider key frames to handle the different content we get served. Bug 1539182 + Bug 1347518 are a couple of the major ones. If OpenH264 must start decoding on an IDR explicitly then I imagine the current code is not going to play nice with it.

Yes, that's exactly the case here.

(In reply to Martin Stránský [:stransky] from comment #13)

The problem is perhaps that we both H264_NAL_IDR_SLICE and H264_NAL_SLICE mark as I_FRAME and consider them as keyframe. But only H264_NAL_IDR_SLICE are the keyframes which we're looking for in seek:

if (nalType == H264_NAL_IDR_SLICE) {
  // IDR NAL.
  return FrameType::I_FRAME;
} else if (nalType == H264_NAL_SEI) {

} else if (nalType == H264_NAL_SLICE) {
  RefPtr<mozilla::MediaByteBuffer> decodedNAL = DecodeNALUnit(p, nalLen);
  if (DecodeISlice(decodedNAL)) {
    return FrameType::I_FRAME;
  }
}

H264_NAL_IDR_SLICE and H264_NAL_SEI must be handled. There's plenty of live content that only provides recovery point, without support for that we wouldn't be able to start decoding to start with as there's just no IDR at all, you can forget about seeking too.

We do not consider a H264_NAL_SLICE to be a keyframe, only I-SLICE (https://searchfox.org/mozilla-central/source/dom/media/platforms/agnostic/bytestreams/H264.cpp#1185-1199)

They are all technically keyframe, you can per spec be able to start decoding from them.

In my experience, you see one type but not the others, so that video where you had trouble seeking would typically have no strict IDR.

Flags: needinfo?(jyavenard)

(In reply to Martin Stránský [:stransky] from comment #11)

Created attachment 9181557 [details]
a simple workaround

There's a simple workaround for the seek error. I wonder what's the original intention here.

The intention is avoid badly muxed content that mark all frames as keyframe regardless of them being a keyframe.

Once upon a time, Chrome MSE stack would only allow to append a buffer if it started with a keyframe (even though there's no such requirement with MSE).
So some sites started to mark all frames as keyframe. Amazon and Twitch in particular did that. They still have such content available.

So while this change may work for that video, I can guarantee you that it would badly break with other videos (it will cause decoding error on both mac and android in particular).

FWIW, Chrome has since adopted the same approach as we do, they totally ignore the keyframe information provided by the container and rely on the H264 bytestream only.

All IDR frames are I-Frame, but not all I-Frame are IDR. Seeking using I-Frame is considered acceptable.

I feel like the only proper option that would cause severe regressions would be to fix OpenH264.

It's a surprising requirements they would have, as WebRTC produced by VideoToolbox encoder (Safari) in particular rarely use IDR

(In reply to Martin Stránský [:stransky] from comment #8)

  1. install openh264 system wide as Mozilla delivers 1.8.1 only (Fedora provides mozilla-openh264-2.1.1 package)

Can you confirm that the 2.x.y versions are actually used? I wouldn't expect us to use the non-GMP version.

Thanks a lot for the reply, looks like it needs more work to get openh264 in shape for media playback.

(In reply to Bryce Seager van Dyk (:bryce) from comment #20)

  1. install openh264 system wide as Mozilla delivers 1.8.1 only (Fedora provides mozilla-openh264-2.1.1 package)

Can you confirm that the 2.x.y versions are actually used? I wouldn't expect us to use the non-GMP version.

Yes, I can confirm that from GMP log.

Filed as openh264 issue: https://github.com/cisco/openh264/issues/3350

Looking at amount of opened issues there I'm rather skeptical of possible fixes from openh264 project. I'm afraid I'll need to create a downstream patch to take keyframes from a container when openh264 decoder is used.

One solution would be to modify the H264 MediaChangeMonitor: https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#28-148

And add a flag that would query if the decoder required strict IDR frame

right now the code ask for a keyframe ; special handling could be done there.

Another thing worth investigating is this bit:
https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#319-320

And the value of the need keyframe, what if this was always set to false ? IIRC it was required with Windows decoder only, without the extra SPS/PPS it would cause an error.

The GMP decoder takes some weird AVCC format where the size is in big-endian order ; I suspect it could be a reason there.

If you set that value to false all the time, how would that go?

How could I reproduce the issue locally?

(In reply to Martin Stránský [:stransky] from comment #21)

Thanks a lot for the reply, looks like it needs more work to get openh264 in shape for media playback.

(In reply to Bryce Seager van Dyk (:bryce) from comment #20)

  1. install openh264 system wide as Mozilla delivers 1.8.1 only (Fedora provides mozilla-openh264-2.1.1 package)

Can you confirm that the 2.x.y versions are actually used? I wouldn't expect us to use the non-GMP version.

Yes, I can confirm that from GMP log.

Do you need to set MOZ_GMP_PATH or link the newly installed lib? I was under the impression we wouldn't load GMPs unless they were in specific locations, but it sounds like I may need to reassess that.

Flags: needinfo?(bvandyk)

Ah, nevermind. I've had a look at some of the different packages and see they're doing this internally. I'd assumed they exposed a generic openh264 to the system, but there are various packages specifically targeted at Gecko. The above Fedora package seems like it will set the GMP path per the source[0].

[0] https://github.com/UnitedRPMs/openh264/blob/5e194f263cecc24f8279e230cc299184a15a28ab/openh264.spec#L86

Priority: -- → P3
Blocks: 1764436
Summary: When decoding video by OpenH264 GMP and position at video stream is changed, decoding fails. → When decoding video by OpenH264 GMP and position at video stream is changed, decoding fails. (Would be fixed by upstreaming Fedora's bug fix: https://src.fedoraproject.org/rpms/firefox/blob/rawhide/f/mozilla-1670333.patch )
Summary: When decoding video by OpenH264 GMP and position at video stream is changed, decoding fails. (Would be fixed by upstreaming Fedora's bug fix: https://src.fedoraproject.org/rpms/firefox/blob/rawhide/f/mozilla-1670333.patch ) → When decoding video by OpenH264 GMP and position at video stream is changed, decoding fails. (Mozilla could take Fedora's downstream fix: https://src.fedoraproject.org/rpms/firefox/blob/rawhide/f/mozilla-1670333.patch )
OS: Unspecified → Linux
Hardware: Unspecified → Desktop
You need to log in before you can comment on or make changes to this bug.