1215115 - Mediarecorder API does not record video on Firefox Android

Status: RESOLVED FIXED

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

Description

[adifran]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.71 Safari/537.36
Firefox for Android

Steps to reproduce:

I checked simpl.info/mediarecorder website on Firefox from Android. It prompts me for audio and video input access. I allowed both camera and mic. 


Actual results:

The demo starts but audio only is recorded, not video


Expected results:

Video had to be recorded and returned as ogv

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

The URL is 404... Can you provide an updated link or source?

Comment 2

[adifran]

Hi Randell, sorry saw this just now.
The url is http://simpl.info/mediarecorder 
If you click it from the url field of this page it will direct you to https://bugzilla.mozilla.org/simpl.info/mediarecorder which does not exist. So type it or copy and paste the exact url.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: MuxOpusIntoWebm.wip.patch

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8732729 [details] [diff] [review]
MuxOpusIntoWebm.wip.patch

Review of attachment 8732729 [details] [diff] [review]:
-----------------------------------------------------------------

I think if we can mux opus into webm, the MediaRecorder api should be work on android. 

It is a wip patch that I try to mux opus into webm container. (opus+vp8 in webm)
Seems works on my android phone.

::: dom/media/encoder/OpusTrackEncoder.cpp
@@ +240,5 @@
>      mLookahead = 0;
>    }
>  
>    // The ogg time stamping and pre-skip is always timed at 48000.
> +  SerializeOpusIdHeader(mChannels, 0, mSamplingRate,

https://wiki.xiph.org/MatroskaOpus
The page says "CodecPrivate consists of the 'OpusHead' packet, identical to the Ogg mapping."
So I put the |meta->mIdHeader in OpusTrackEncoder::GetMetadata|  into |mEbmlComposer->SetAudioCodecPrivateData|. 
Then I hit the |codecDelay != pre-skip in OpusDataDecoder::Init()| when play the blob.
So set the pre-skip to 0 as a workaround. 

Ralph: Do you know how to fix this? And any suggestion about opus in webm?

Comment 6

[Maire Reavy [:mreavy]]

Thanks, Benjamin, for your patch.  I think your approach of recording Opus + VP8 in webm is a good one.  (I believe there is a lack of fixed point Vorbis support on Android -- so using Vorbis on Android is a non-starter.)  I'd actually prefer to record VP8 + Opus (instead of Vorbis) on Desktop as well.

With this in mind, let's plan to land this when we have Opus + VP8 playback support (which I believe Ralph is working on). Would you like to own this bug? 

I've talked with Jesup, and he's fine with giving this bug to you.  If you can't take this bug, that's ok.  Just let me know what you want to do.

Also FYI: I've moved this from a P1 to a P2 (in part because we're gated on Opus + VP8 playback support), but I still very much want this to land as soon as it's ready and playback support is ready.  Thanks!

Comment 7

[Ralph Giles (:rillian)]

(In reply to Benjamin Chen [:bechen] from comment #5)

> Then I hit the |codecDelay != pre-skip in OpusDataDecoder::Init()| when play
> the blob.

You need to convert the pre-skip value from the private header from "samples at 48000 Hz" to "nanoseconds" and emit a CodecDelay element ([56][AA]) with that value inside the TrackEntry element in the file header.

https://matroska.org/technical/specs/index.html#CodecDelay

You also need to emit a SeekPreRoll element in the same header (standard value of 80 ms also converted to nanoseconds).

Let me know if you have any other issues!

Comment 8

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

Review commit: https://reviewboard.mozilla.org/r/42435/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42435/

Comment 9

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1215115 - part1: Replace the vorbis by opus in MediaEncoder and also reomve the VorbisTrackEncoder files. r=rillian

Review commit: https://reviewboard.mozilla.org/r/42437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42437/

Comment 10

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1215115 - part2: Mux opus into webm, remove bitdepth. r=rillian

Review commit: https://reviewboard.mozilla.org/r/42439/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42439/

Comment 11

[Benjamin Chen [:bechen]]

Attachment: MozReview Request: Bug 1215115 - part3: Fix gtest. Remove TestVorbisTrackEncoder.cpp. r=rillian

Review commit: https://reviewboard.mozilla.org/r/42441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42441/

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

https://reviewboard.mozilla.org/r/42435/#review39245

This should probably be last, not first, given the build breakage. Looks fine to me, but should have build peer review as well.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8734776 [details]
MozReview Request: Bug 1215115 - part1: Replace the vorbis by opus in MediaEncoder and also reomve the VorbisTrackEncoder files. r=rillian

https://reviewboard.mozilla.org/r/42437/#review39247

r=me with comment and build failures addressed.

::: dom/media/encoder/moz.build:36
(Diff revision 1)
> -    EXPORTS += ['VorbisTrackEncoder.h',
> +    EXPORTS += ['VP8TrackEncoder.h',
> -                'VP8TrackEncoder.h',
>      ]
> -    UNIFIED_SOURCES += ['VorbisTrackEncoder.cpp',
> +    UNIFIED_SOURCES += ['VP8TrackEncoder.cpp',
> -                        'VP8TrackEncoder.cpp',
>      ]

You should also remove the `VorbisTrackEncoder.*` source files if you're not building them any more. They'll still be available in the repository history if anyone wants the code later.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8734777 [details]
MozReview Request: Bug 1215115 - part2: Mux opus into webm, remove bitdepth. r=rillian

https://reviewboard.mozilla.org/r/42439/#review39271

Looks like a reasonable start, but I'd like to see it again. Also, TestVorbisTrackEncoder should be rewritten to be TestOpusTrackEncoder.

::: dom/media/encoder/OpusTrackEncoder.cpp:235
(Diff revision 1)
>  
>    RefPtr<OpusMetadata> meta = new OpusMetadata();
> +  meta->mChannels = mChannels;
> +  meta->mSamplingFrequency = mSamplingRate;
> +  // TODO: fix me
> +  meta->mBitDepth = 32;

mBitDepth doesn't seem to actually be used anywhere. Try removing it entirely; Take it out of `VorbisMetadata` and `OpusMetadata`, remove the argument from `EbmlComposer::SetAudioConfig()` and see if anything complains.

::: dom/media/webm/EbmlComposer.cpp:59
(Diff revision 1)
> +            // Details in OpusTrackEncoder.cpp.
> +            uint64_t codecDelay =
> +              (uint64_t)LittleEndian::readUint16(mCodecPrivateData.Elements() + 10)
> +              * PR_NSEC_PER_SEC / 48000;
> +            // Fixed 80ms, convert into nanoseconds.
> +            uint64_t seekPreRoll = 80 * PR_USEC_PER_SEC;

`80 * PR_NSEC_PER_MSEC`

::: dom/media/webm/EbmlComposer.cpp:149
(Diff revision 1)
>      Ebml_SerializeUnsigned(&ebml, Timecode, mClusterTimecode);
>      mFlushState |= FLUSH_CLUSTER;
>    }
>  
> -  bool isVorbis = (frameType == EncodedFrame::FrameType::VORBIS_AUDIO_FRAME);
> +  bool isOpus = (frameType == EncodedFrame::FrameType::OPUS_AUDIO_FRAME);
>    short timeCode = aFrame->GetTimeStamp() / PR_USEC_PER_MSEC - mClusterTimecode;

You may need to add CodecDelay here, depending on how the timestamps get set on the incoming frame.

Comment 15

[Ralph Giles (:rillian)]

Comment on attachment 8734778 [details]
MozReview Request: Bug 1215115 - part3: Fix gtest. Remove TestVorbisTrackEncoder.cpp. r=rillian

https://reviewboard.mozilla.org/r/42441/#review39331

r=me with comment addressed.

::: dom/media/gtest/moz.build:35
(Diff revision 1)
>      ]
>  
>  if CONFIG['MOZ_WEBM_ENCODER']:
>      UNIFIED_SOURCES += [
>          'TestVideoTrackEncoder.cpp',
> -        'TestVorbisTrackEncoder.cpp',
> +        #'TestVorbisTrackEncoder.cpp',

Don't comment-out lines like this, just delete them.

Or replace this with TestOpusTrackEncoder.cpp, really.

Comment 16

[adifran]

Hi there. If I understand well, you are setting things up so that MediaRecorder API output has now Opus encoding for audio. Please note that this will create a problem with Web Audio API, which does not read Opus. The problem is already known on Chrome. They are working on it as per this bug https://bugs.chromium.org/p/chromium/issues/detail?id=482934

It would be bad to have MediaRecorder API and Web Audio API not able to talk to each other, I mean, I think it is obvious to everybody.

So please, just take this into account. If I did not understand well your last comments and you are fixing something else, please disregard this comment.

Comment 17

[Benjamin Chen [:bechen]]

Comment on attachment 8734776 [details]
MozReview Request: Bug 1215115 - part1: Replace the vorbis by opus in MediaEncoder and also reomve the VorbisTrackEncoder files. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42437/diff/1-2/

Comment 18

[Benjamin Chen [:bechen]]

Comment on attachment 8734777 [details]
MozReview Request: Bug 1215115 - part2: Mux opus into webm, remove bitdepth. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42439/diff/1-2/

Comment 19

[Benjamin Chen [:bechen]]

Comment on attachment 8734778 [details]
MozReview Request: Bug 1215115 - part3: Fix gtest. Remove TestVorbisTrackEncoder.cpp. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42441/diff/1-2/

Comment 20

[Benjamin Chen [:bechen]]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42435/diff/1-2/

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8734777 [details]
MozReview Request: Bug 1215115 - part2: Mux opus into webm, remove bitdepth. r=rillian

https://reviewboard.mozilla.org/r/42439/#review39877

::: dom/media/webm/EbmlComposer.h:71
(Diff revision 2)
>    // The cluster length position.
>    uint64_t mClusterLengthLoc;
>    // Audio codec specific header data.
>    nsTArray<uint8_t> mCodecPrivateData;
> +  // Codec delay in nanoseconds.
> +  uint64_t mCodecDelay;

Since the inialization in conditional in GenerateHeader() you need to set this to zero in the EmblComposer constructor at the end of EbmlComposer.cpp.

Comment 22

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/42441/#review40011

::: dom/media/test/mochitest.ini
(Diff revision 2)
>  [test_mediarecorder_record_audiocontext_mlk.html]
>  tags=msg
>  [test_mediarecorder_record_audionode.html]
>  tags=msg
>  [test_mediarecorder_record_canvas_captureStream.html]
> -skip-if = (android_version < '17' || toolkit == 'android') # Android/Gonk before SDK version 17 does not have the OMX Encoder API and Fennec does not support video recording

I will not enable these 3 tests because I found they always fail at all platforms.
Seems that remove the |skip-if = (android_version < '17' || toolkit == 'android')| also enable the testcase at other platforms. (I guess other platforms have a android_version which is smaller than 17...)

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

https://reviewboard.mozilla.org/r/42435/#review40133

If this is going to be enabled by default on Firefox desktop and Firefox for Android I submit that it should just be on-by-default in configure.

Comment 24

[Benjamin Chen [:bechen]]

Comment on attachment 8734777 [details]
MozReview Request: Bug 1215115 - part2: Mux opus into webm, remove bitdepth. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42439/diff/2-3/

Comment 25

[Benjamin Chen [:bechen]]

Comment on attachment 8734778 [details]
MozReview Request: Bug 1215115 - part3: Fix gtest. Remove TestVorbisTrackEncoder.cpp. r=rillian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42441/diff/2-3/

Comment 26

[Benjamin Chen [:bechen]]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42435/diff/2-3/

Comment 27

[Benjamin Chen [:bechen]]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42435/diff/3-4/

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 8734775 [details]
MozReview Request: Bug 1215115 - part4: Enable MOZ_WEBM_ENCODER by default. r=ted

https://reviewboard.mozilla.org/r/42435/#review40293

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d41311af46d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf791204fc36
https://hg.mozilla.org/integration/mozilla-inbound/rev/908acebd5af4
https://hg.mozilla.org/integration/mozilla-inbound/rev/11cfc06439f9

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d41311af46d
https://hg.mozilla.org/mozilla-central/rev/bf791204fc36
https://hg.mozilla.org/mozilla-central/rev/908acebd5af4
https://hg.mozilla.org/mozilla-central/rev/11cfc06439f9