1808680 - Crash in [@ mozilla::EbmlComposer::WriteSimpleBlock]

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect, P2)

Description

[Andreas Pehrson [:pehrsons]]

See bug 1702483 comment 14. This assert means that we have encoded a non-keyframe that has a timestamp more than 32.767 seconds after the most recent keyframe, and we fail to write this to the webm container because the timestamp (milliseconds as an int16_t) would wrap.

But VP8TrackEncoder has logic to force a keyframe when we encode a frame whose timestamp is >10s after the previously encoded keyframe. From 10s to 32s there is definitely no race going on. Unclear where the bug is.

+++ This bug was initially created as a clone of Bug #1702483 +++

Crash report: https://crash-stats.mozilla.org/report/index/1a4160d4-3d35-44d5-ba39-39ced0210401

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(timeCode <= 32767)

Top 10 frames of crashing thread:

0 xul.dll mozilla::EbmlComposer::WriteSimpleBlock dom/media/webm/EbmlComposer.cpp:104
1 xul.dll mozilla::WebMWriter::WriteEncodedTrack dom/media/webm/WebMWriter.cpp:24
2 xul.dll mozilla::Muxer::Mux dom/media/encoder/Muxer.cpp:176
3 xul.dll mozilla::Muxer::GetData dom/media/encoder/Muxer.cpp:87
4 xul.dll mozilla::MediaEncoder::GetEncodedData dom/media/encoder/MediaEncoder.cpp:749
5 xul.dll mozilla::MediaEncoder::Extract dom/media/encoder/MediaEncoder.cpp:954
6 xul.dll mozilla::MediaEncoder::RequestData dom/media/encoder/MediaEncoder.cpp:882
7 xul.dll mozilla::MediaEncoder::MaybeExtractOrGatherBlob dom/media/encoder/MediaEncoder.cpp:921
8 xul.dll mozilla::MediaEncoder::OnEncodedVideoPushed dom/media/encoder/MediaEncoder.cpp:910
9 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::layers::APZCTreeManager>, void  xpcom/threads/nsThreadUtils.h:1201

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 content process crashes on beta

:pehrsons, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 2

[Andreas Pehrson [:pehrsons]]

I still don't understand how this happens, but ok, let's try to gather more data.

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1808680 - Report some input variables to get clues on the crash. r?padenot!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d43b691becf
Report some input variables to get clues on the crash. r=padenot

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1d43b691becf

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signatures linked to this bug are not in the topcrash signatures anymore.

For more information, please visit auto_nag documentation.

Comment 7

[Donal Meehan [:dmeehan]]

:pehrsons this is in a leave-open state, there are still crash reports in nightly.
Can you investigate?

Comment 8

[Donal Meehan [:dmeehan]]

Missing an NI for Comment 7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

From one of the more recent reports:

Invalid cluster timecode! audio=1, video=1, timeCode=32779ms, currentClusterTimecode=266967ms

Comment 10

[Andreas Pehrson [:pehrsons]]

There is logic in VideoTrackEncoder to force a keyframe if 10 seconds has passed since the last keyframe.

But here we see ~33 seconds pass without a key frame when writing the container.

There is one case where we pass a frame to the muxer with a part of its duration unaccounted for. It's when we skip a frame because encoding is slow.

The error here is accumulative but reset whenever we observe the encoder spitting out a keyframe.
I guess if we've built up a long queue of frames to encode (30+ seconds), and encoding is really really slow (so we skip 20+ seconds worth of frames in this queue while encoding <10 seconds worth of them) then we'd bypass the force-keyframe-after-10s check and could end up with this crash.

We should fix this, and hopefully it has bearing on the crash stats.

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1808680 - In VP8TrackEncoder account for skipped frames when deciding whether to force a keyframe. r?padenot!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1160526391b4
In VP8TrackEncoder account for skipped frames when deciding whether to force a keyframe. r=padenot

Comment 13

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1160526391b4

Comment 14

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9320966 [details]
Bug 1808680 - In VP8TrackEncoder account for skipped frames when deciding whether to force a keyframe. r?padenot!

Beta/Release Uplift Approval Request

• User impact if declined: This is not a verified fix to the crash because we only know from crash-stats whether it worked. But if declined we'll for certain have the same crash rate in 111 as we had in 110.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple. This adds a missing case of growing an integer in some bookkeeping logic. It could grow faster but should not grow further than before. No changes to lifetimes, threading or the like.
• String changes made/needed:
• Is Android affected?: No

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9320966 [details]
Bug 1808680 - In VP8TrackEncoder account for skipped frames when deciding whether to force a keyframe. r?padenot!

111 is now in RC, changing the beta uplift request to a release uplift request. This will be included for consideration for a later 111 release build.
If it is not intended to request for release, please cancel the request.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:pehrsons, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9320966 [details]
Bug 1808680 - In VP8TrackEncoder account for skipped frames when deciding whether to force a keyframe. r?padenot!

Approved for 111.0.1 and Fenix/Focus 111.1.0

Comment 18

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/ca25d5e47b29

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Looks like we're still seeing crashes here :(
https://crash-stats.mozilla.org/report/index/c85a9dd0-3152-4ec3-93d1-195470230324

Invalid cluster timecode! audio=1, video=1, timeCode=32781ms, currentClusterTimecode=232885ms

https://crash-stats.mozilla.org/report/index/ac950dbc-cfeb-42ef-917e-1c7bf0230326

Invalid cluster timecode! audio=1, video=1, timeCode=32787ms, currentClusterTimecode=14279ms

https://crash-stats.mozilla.org/report/index/6a8b03b2-3ca1-4769-ac35-e9e250230323

Invalid cluster timecode! audio=1, video=1, timeCode=32780ms, currentClusterTimecode=6ms

Comment 20

[Andreas Pehrson [:pehrsons]]

Let's follow up with this in a separate bug. Closing as fixed since we still fixed something, just not the thing we were hoping for.