Closed Bug 1272371 Opened 8 years ago Closed 7 years ago

Webm video recorded with MediaRecorder cannot be played more than once

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
46 Branch
Platform:
x86_64
Windows 8.1
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: it.blackdog, Assigned: bryce)

Details

Attachments

(4 files)

multiplex.patch
797 bytes, patch
bryce
: feedback+
Details | Diff | Splinter Review
Bug 1272371 - Add mochitest to test looped playback of mediarecorder recordings.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1272371 - Update multiplexed input stream seek behaviour.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1272371 - Add gtest to cover nsMultiplexInputStream set seek.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36

Steps to reproduce:

I created plunker for bug demo: https://plnkr.co/edit/EbV9alDfpSMELfep4Yfh

1) Open https://plnkr.co/edit/EbV9alDfpSMELfep4Yfh page
2) Click "Run" button to run sample
3) Allow access to web camera
4) Recorded video will be displayed on rigth side of page
5) Record few seconds of video 
6) Press "Stop" button
7) Recorded video will automatically played inside video element that was used to capture stream from camera
8) After video playback is finished press "play" button inside  video element


Actual results:

Video element became gray and loader indicator appears inside it, recorded video not playing 


Expected results:

Recorded video must be played second time.

I also created another plunker to demonstrate this bug:  https://plnkr.co/edit/hU3UkESutPpQlIln0mlF?p=preview

If  you upload webm video recorded with Chrome (you can use this demo to record webm file with Chrome https://webrtc.github.io/samples/src/content/getusermedia/record/ ), and try to play this video more than once, results will be the same.
Notice if you use example webm video from Wiki (https://en.wikipedia.org/wiki/WebM) for second test -  this video will be played normaly, replay and playback time position change works absolutely fine.
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
Component: Untriaged → Audio/Video: Recording
Product: Firefox → Core
I think it's fixed in Nightly. Could you test, please? https://nightly.mozilla.org/
Flags: needinfo?(it.blackdog)
Just tested with Nightly version.
For first test case:
 After video was reocorded and atomatically played by test script I click on play button inside video element(try play video second time), now video element does not get loading indicator, but recorded video is not played for second time.

For second test case:
 Seems issue is fixed (video recorded from Chrome, and that I recorded by FF earlier) played fine after second playback and so on.
Flags: needinfo?(it.blackdog)
Whiteboard: [issue fixed on latest Nightly 2016-05-12]
Please pardon me for my impatience, but i think that issue is not 100% fixed on latest Nightly 2016-05-12.

Primary test case (https://plnkr.co/edit/EbV9alDfpSMELfep4Yfh) still fail to play recorded video after first playback.
This may be outside mediarecorder per-se (bryce?)

I'll note that if no parameter is passed to mediarecorder.start(), it works fine, or if you hit Stop before the first slice pops out (likely need to up 500 to a larger value).  So the problem is in replaying a blob source that was created from multiple other blobs.
Status: UNCONFIRMED → NEW
Component: Audio/Video: Recording → Audio/Video: Playback
Ever confirmed: true
Flags: needinfo?(bvandyk)
Whiteboard: [issue fixed on latest Nightly 2016-05-12]
I've had a look and am able to repro. If I save the file locally I am unable to repro though. I've tried serving it different ways locally, and all seem to allow for replay. I'm currently engaged in trying to get some other stuff out the door, but I've notified the cavalry and expect this to be looked into. If there's no follow up in a reasonable time frame, please notify me.
Flags: needinfo?(bvandyk)
Blake: Is this something your team can help with? You are the cavalry riding to our rescue it seems. :)
Flags: needinfo?(bwu)
(In reply to Chris Pearce (:cpearce) from comment #6)
> Blake: Is this something your team can help with? You are the cavalry riding
> to our rescue it seems. :)
I believe Alfredo is the cavalry you need!
Alfredo, 
Could you help on this?
Thanks!
Flags: needinfo?(bwu) → needinfo?(ayang)
Assignee: nobody → ayang
Flags: needinfo?(ayang)
This looks like bug 657791.
Don't work with nightly build.



[MediaPlayback #2]: D/MediaFormatReader MediaFormatReader(13764c000)::Seek: aTarget=(0)
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::GetBuffered: Duration: 0.000000 StartTime: 0.000000
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::GetBuffered: add range 0.000000-5.982000
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12a78dba0)::Reset: Seek to start point: 0.000000
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::SeekInternal: Seek Target: 0.000000
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::SeekInternal: SeekPreroll: 0.080000 StartTime: 0.000000 Adjusted Target: 0.000000
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::SeekInternal: track_seek for track 0 to 0.000000 failed, r=-1
[MediaPlayback #2]: D/Nestegg 0x133cc7640 [Nestegg-DBG] seek: parsing cluster elements
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12926a000)::SeekInternal: and nestegg_offset_seek to 325 failed
[MediaPlayback #2]: D/WebMDemuxer WebMDemuxer(12a78dba0)::SetNextKeyFrameTime: Couldn't determine next keyframe time  (0 queued)
Ok, I think I found the problem. The blob can't seek back to 0 if timeslice is not 0. If you use MediaRecorder.start() without timeslice. The problem is gone at nightly build.

I'll keep checking what's wrong in timeslice implementation.
I found the blob can't seek back to head after playing. If we always create a new src via createObjectURL such as https://plnkr.co/edit/HyWiXSORiEUBYAl0nJTE when video.onended(), it is ok to replay.

I believe there is nothing wrong in media playback or media recorder.
Flags: needinfo?(bwu)
Could you elaborate more about where you see that we cannot seek back to head?
Flags: needinfo?(bwu)
Mass change P2 -> P3
Priority: P2 → P3
I'm seeing this issue again recently with my work on MediaRecorder and have had a couple of other persons bring it to my attention. Any updates?
This can be tested at: https://simpl.info/mediarecorder/ One can record a video and then attempt to play it back more than once. This will not currently work.

Looking into the cause: since the created blobs don't have any cues seeking falls back to our offset based seeking approach[1]. This appears to be giving a correct offset. I'm basing this off downloading the same video and inspecting it via mkvinfo.

This offset will be used by the WebMDemuxer during the following calls (trimmed):

> WebMDemuxer::DemuxPacket(mozilla::TrackInfo::TrackType aType, RefPtr<mozilla::NesteggPacketHolder> & aPacket) Line 910
> WebMDemuxer::NextPacket(mozilla::TrackInfo::TrackType aType, RefPtr<mozilla::NesteggPacketHolder> & aPacket) Line 890
> WebMDemuxer::GetNextPacket(mozilla::TrackInfo::TrackType aType, mozilla::MediaRawDataQueue * aSamples) Line 580
> WebMTrackDemuxer::NextSample(RefPtr<mozilla::MediaRawData> & aData) Line 1115
> WebMTrackDemuxer::SetNextKeyFrameTime() Line 1184
> WebMTrackDemuxer::Reset() Line 1225

DemuxPacket calls back into nestegg to read the next packet. Nestegg will in turn call into webmdemux_read[2], which skips the media source case and calls into Read(). Call stack (trimmed):

> FileMediaResource::ReadAt(__int64 aOffset, char * aBuffer, unsigned int aCount, unsigned int * aBytes) Line 1443
> MediaResourceIndex::ReadAt(__int64 aOffset, char * aBuffer, unsigned int aCount, unsigned int * aBytes) Line 1653
> MediaResourceIndex::Read(char * aBuffer, unsigned int aCount, unsigned int * aBytes) Line 1638
> webmdemux_read(void * aBuffer, unsigned int aLength, void * aUserData) Line 75

FileMediaResource::ReadAt[3] contains calls into UnsafeSeek and UnsafeRead. UnsafeSeek calls into nsMultiplexInputStream::Seek[4]. UnsafeRead calls into nsMultiplexInputStream::Read[5] and this appears to be where things go badly. The input stream stores a mCurrentStream, and this is checked during the read[6]. However, this value always appears to be 2 in my testing. If there are 2 streams then 2 is always out of bounds of the check and we will never read from our stream. Thus we return 0 bytes and nestegg fails its read.

I'm not sure what is responsible for setting mCurrentStream, it appears that nsMultiplexInputStream::Seek should do so at least some of the time. I'm going to investigate further, but am info dumping in case I don't end up finishing this (though as of writing I intend to).

[1]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webm/WebMDemuxer.cpp?q=WebMDemuxer.cpp&redirect_type=direct#987
[2]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webm/WebMDemuxer.cpp?q=WebMDemuxer.cpp&redirect_type=direct#56
[3]: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaResource.cpp?q=MediaResource.cpp&redirect_type=direct#1433
[4]: https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp?q=nsMultiplexInputStream.cpp&redirect_type=direct#415
[5]: https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp?q=nsMultiplexInputStream.cpp&redirect_type=direct#259
[6]: https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsMultiplexInputStream.cpp?q=nsMultiplexInputStream.cpp&redirect_type=direct#278
(In reply to Bryce Van Dyk (:SingingTree) from comment #14)
> I'm seeing this issue again recently with my work on MediaRecorder and have
> had a couple of other persons bring it to my attention. Any updates?

Sorry, I don't check this for a while. Feel free to take it.
Assignee: ayang → nobody
(In reply to Alfredo Yang (:alfredo) from comment #16)
> Sorry, I don't check this for a while. Feel free to take it.

Cheers, will do!
Assignee: nobody → bvandyk
Attached patch multiplex.patch — — Splinter Review 
You are right! nsMultiplexInputStream is buggy. I didn't push this patch to try. But I suspect that the fix is something like that.
Attachment #8870744 - Flags: feedback?(bvandyk)
Comment on attachment 8870744 [details] [diff] [review]
multiplex.patch

Thank you for taking a look. This appears to resolve the issue in my local testing. I'd like to write a test to cover the case where this is an issue in media recorder, then land these changes and the test. Are you okay with me taking and running with this patch?
Flags: needinfo?(amarchesini)
Attachment #8870744 - Flags: feedback?(bvandyk) → feedback+
Yes please. Thanks for adding a test!
Flags: needinfo?(amarchesini)
Comment on attachment 8872193 [details]
Bug 1272371 - Update multiplexed input stream seek behaviour.

https://reviewboard.mozilla.org/r/143644/#review147390

Please add a gtest for this behavior.  A mochitest is good, but an additional gtest would be good to have too, in case the underlying bits for the mochitest change to not using this code.

::: xpcom/io/nsMultiplexInputStream.cpp:503
(Diff revision 1)
>          NS_ASSERTION(remaining == streamPos, "Huh?");
>          remaining = 0;

Can you MOZ_ASSERT that `remaining != 0` here, before you set `remaining = 0`?
Attachment #8872193 - Flags: review?(nfroyd)
Comment on attachment 8872192 [details]
Bug 1272371 - Add mochitest to test looped playback of mediarecorder recordings.

https://reviewboard.mozilla.org/r/143642/#review147478

::: dom/media/test/test_mediarecorder_playback_can_repeat.html:53
(Diff revision 1)
> +    // This avoids the case where we have only 1 frame in the output, which breaks
> +    // looping.

Breaks looping expectedly or unexpectedly?

::: dom/media/test/test_mediarecorder_playback_can_repeat.html:73
(Diff revision 1)
> +  mediaRecorder.onstop = () => {
> +    info("Got 'stop' event");
> +
> +    ok(blob, "Should have gotten a data blob");
> +    let video = document.getElementById("recorded-video");
> +    video.src = URL.createObjectURL(blob);

Use srcObject.
Attachment #8872192 - Flags: review?(pehrson) → review+
Comment on attachment 8872192 [details]
Bug 1272371 - Add mochitest to test looped playback of mediarecorder recordings.

https://reviewboard.mozilla.org/r/143642/#review147478

> Breaks looping expectedly or unexpectedly?

Fixed the comment here to make it clear this breakage would be expected.

> Use srcObject.

Not sure this can be done with a blob, please let me know if I'm overlooking something.
Comment on attachment 8872192 [details]
Bug 1272371 - Add mochitest to test looped playback of mediarecorder recordings.

https://reviewboard.mozilla.org/r/143642/#review147478

> Not sure this can be done with a blob, please let me know if I'm overlooking something.

Sorry, too used to MediaStream.
Comment on attachment 8873272 [details]
Bug 1272371 - Add gtest to cover nsMultiplexInputStream set seek.

https://reviewboard.mozilla.org/r/144730/#review148820

Thank you!

::: xpcom/tests/gtest/TestMultiplexInputStream.cpp:27
(Diff revision 1)
> +  NS_NewCStringInputStream(getter_AddRefs(inputStream1), buf1);
> +  NS_NewCStringInputStream(getter_AddRefs(inputStream2), buf2);
> +  NS_NewCStringInputStream(getter_AddRefs(inputStream3), buf3);

Please check the `nsresult` return value from these for `NS_SUCCEEDED`, just for completeness.
Attachment #8873272 - Flags: review?(nfroyd) → review+
Comment on attachment 8872193 [details]
Bug 1272371 - Update multiplexed input stream seek behaviour.

https://reviewboard.mozilla.org/r/143644/#review148822
Attachment #8872193 - Flags: review?(nfroyd) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 325d2ef3ca76 -d 13e2d926f0e6: rebasing 399739:325d2ef3ca76 "Bug 1272371 - Add mochitest to test looped playback of mediarecorder recordings. r=pehrsons"
merging dom/media/test/mochitest.ini
warning: conflicts while merging dom/media/test/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb9b5fe6b4c7
Add mochitest to test looped playback of mediarecorder recordings. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/c612f1bb40a4
Add gtest to cover nsMultiplexInputStream set seek. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/f30bf8724ee9
Update multiplexed input stream seek behaviour. r=froydnj
Setting qe-verify- for manual verification, since this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: