1696794 - MediaRecorder audio doesn't record all the duration

Status: VERIFIED FIXED

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

Description

[Simon GALAIS]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:88.0) Gecko/20100101 Firefox/88.0

Steps to reproduce:

I record 10 seconds of audio , and read it

Actual results:

At the play , I have only 3 seconds of sound

Expected results:

I should have 10 seconds of audio on playing the audio.
Same code works on Firefox and chromium , but not on Nightly ...

Comment 1

[Simon GALAIS]

Can be test on : https://mozdevs.github.io/MediaRecorder-examples/record-live-audio.html

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::Audio/Video: Playback' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[John Lin [:jhlin][:jolin]]

I don't have a Linux box to check it locally but this looks like a MediaRecorder issue. Moving it to the recorder component.
I can confirm that it happens in Nightly on macOS too.

Comment 4

[Karl Tomlinson (:karlt)]

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1c9c0c0c5fd05a1cf1eb611fbfa90de4953d4273&tochange=af570005d5939fe037c87a285856597ba502a95b

Comment 5

[Andreas Pehrson [:pehrsons]]

I'd say this is the regressor: https://hg.mozilla.org/mozilla-central/rev/bb3690c52cb92637d097817682902a5165262ddf
In particular the logic that calls Extract() only every second.

We'd only get one page out of OggWriter per GetContainerData() call. Flawed behavior perhaps, but it worked fine prior to bug 1464268, because we'd call it very often. With bug 1464268 we call it once per second. At 128kbps, one page of 4kB is 250ms. Thus with default audio bitrate and no timeslice we'd only get the first 25% of all pages by the end of the recording.

Workarounds:

• Set the timeslice at <250ms (with default bitrate - 128kbps)
• Set audio bitrate to <32kbps (with no timeslice - extracting pages every second)

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1696794 - Add gtest. r?bryce!

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1696794 - Drain all full pages in OggWriter::GetContainerData. r?bryce!

With bug 1464268 and friends the ContainerWriters starting being called once per
second to get written data out. OggWriter was only able to handle getting one
page out per call, so if we had written N pages to OggWriter while processing a
second worth of data, it'd still have N-1 pages in a buffer. That led to
MediaRecorder writing much too short files when recording audio/ogg.

This patch fixes this properly in OggWriter by extracting all available pages in
GetContainerData().

Comment 8

[Andreas Pehrson [:pehrsons]]

Comment on attachment 9210206 [details]
Bug 1696794 - Drain all full pages in OggWriter::GetContainerData. r?bryce!

Beta/Release Uplift Approval Request

• User impact if declined: Using MediaRecorder for audio-only recordings with otherwise default settings results in only the first ~25% of the recording being saved.
• Is this code covered by automated tests?: Yes
• 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 change in code with a very narrow use-case. Would only affect this functionality that is already broken (see the impact field on how).
• String changes made/needed:

Comment 9

[Andreas Pehrson [:pehrsons]]

I should say, the simplest workaround would be setting the mime type to audio/webm.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/428a79637282
Add gtest. r=bryce
https://hg.mozilla.org/integration/autoland/rev/c66f0d85eed2
Drain all full pages in OggWriter::GetContainerData. r=bryce

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (bug 1696794) for bustage complaining about gtest.h.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=c66f0d85eed25798de54f7309ac7d6fd9fdb2332&searchStr=build&selectedTaskRun=ezTucJNHRwq3vd5TePv3Mw.0&tochange=2c5cce48931114735d687cf9458482683e3e32f7

Backout link: https://hg.mozilla.org/integration/autoland/rev/2c5cce48931114735d687cf9458482683e3e32f7

Failure log: https://treeherder.mozilla.org/logviewer?job_id=333796669&repo=autoland&lineNumber=10570

[task 2021-03-19T16:41:50.885Z] 16:41:50     INFO -  make[4]: Entering directory '/builds/worker/workspace/obj-build/dom/media/gtest'
[task 2021-03-19T16:41:50.893Z] 16:41:50     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ --sysroot /builds/worker/fetches/sysroot -std=gnu++17 -o Unified_cpp_dom_media_gtest2.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fstack-clash-protection -DNDEBUG=1 -DTRIMMED=1 -DHAVE_UINT64_T -DWEBRTC_POSIX -DWEBRTC_BUILD_LIBEVENT -DWEBRTC_LINUX -DENABLE_SET_CUBEB_BACKEND -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/dom/media/gtest -I/builds/worker/workspace/obj-build/dom/media/gtest -I/builds/worker/checkouts/gecko/dom/media/webrtc/common -I/builds/worker/checkouts/gecko/third_party/libwebrtc -I/builds/worker/checkouts/gecko/third_party/libwebrtc/webrtc -I/builds/worker/checkouts/gecko/gfx/2d -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/checkouts/gecko/dom/media -I/builds/worker/checkouts/gecko/dom/media/encoder -I/builds/worker/checkouts/gecko/dom/media/gmp -I/builds/worker/checkouts/gecko/dom/media/mp4 -I/builds/worker/checkouts/gecko/dom/media/platforms -I/builds/worker/checkouts/gecko/dom/media/platforms/agnostic -I/builds/worker/checkouts/gecko/dom/media/webrtc -I/builds/worker/checkouts/gecko/security/certverifier -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wno-unknown-warning-option -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -fno-aligned-new -fcrash-diagnostics-dir=/builds/worker/artifacts -fno-exceptions -fno-strict-aliasing -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -Wno-error=shadow -Wno-inconsistent-missing-override -Wno-unused-private-field -fexperimental-new-pass-manager  -MD -MP -MF .deps/Unified_cpp_dom_media_gtest2.o.pp   Unified_cpp_dom_media_gtest2.cpp
[task 2021-03-19T16:41:50.894Z] 16:41:50     INFO -  In file included from Unified_cpp_dom_media_gtest2.cpp:2:
[task 2021-03-19T16:41:50.894Z] 16:41:50     INFO -  In file included from /builds/worker/checkouts/gecko/dom/media/gtest/TestMediaDataEncoder.cpp:6:
[task 2021-03-19T16:41:50.894Z] 16:41:50    ERROR -  /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:1445:11: error: comparison of integers of different signs: 'const unsigned long' and 'const int' [-Werror,-Wsign-compare]
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -    if (lhs == rhs) {
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -        ~~~ ^  ~~~
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -  /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:1473:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned long, int>' requested here
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -      return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -             ^
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -  /builds/worker/checkouts/gecko/dom/media/gtest/TestOggWriter.cpp:59:3: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<unsigned long, int>' requested here
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -    EXPECT_EQ(inputBytes, 16000);
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -    ^
[task 2021-03-19T16:41:50.895Z] 16:41:50     INFO -  /builds/worker/workspace/obj-build/dist/include/gtest/gtest.h:1969:63: note: expanded from macro 'EXPECT_EQ'
[task 2021-03-19T16:41:50.896Z] 16:41:50     INFO -                        EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
[task 2021-03-19T16:41:50.896Z] 16:41:50     INFO -                                                                ^
[task 2021-03-19T16:41:50.896Z] 16:41:50     INFO -  1 error generated.
[task 2021-03-19T16:41:50.896Z] 16:41:50    ERROR -  make[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:674: Unified_cpp_dom_media_gtest2.o] Error 1
[task 2021-03-19T16:41:50.896Z] 16:41:50     INFO -  make[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/media/gtest'
[task 2021-03-19T16:41:50.896Z] 16:41:50    ERROR -  make[3]: *** [/builds/worker/checkouts/gecko/config/recurse.mk:72: dom/media/gtest/target-objects] Error 2
[task 2021-03-19T16:41:50.896Z] 16:41:50     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ef08110a07f
Add gtest. r=bryce
https://hg.mozilla.org/integration/autoland/rev/663cc2bafe0a
Drain all full pages in OggWriter::GetContainerData. r=bryce

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4ef08110a07f
https://hg.mozilla.org/mozilla-central/rev/663cc2bafe0a

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9210205 [details]
Bug 1696794 - Add gtest. r?bryce!

It's too late for 87.0. Keeping on the radar for a dot release.

Comment 16

[Cornel Ionce [:noni]]

Reproduced the initial issue on Windows 10 x64 using Firefox 87.0 with the mention that only 4 seconds out of 12 were available.

Confirming that the entire recorded duration is correctly playing back on Firefox 88.0b5 (buildID 20210330185720) on Windows 10 x64 and Ubuntu 20.04.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9210205 [details]
Bug 1696794 - Add gtest. r?bryce!

88 is in RC now and there are no plans for an 87 dot release.