792274 - Don't setup mediastreams in RecreateDecodedStream() if we did not have a mediastream before.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

Attachment: Patch

This changes the code path used after a seek from the normal: reader -> media queue -> output to use an intermediate step using MediaStreams, which is unnecessary.

As discussed on irc, this patches returns early in nsBuiltinDecoder::RecreateDecodedStream() if there is no output streams.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Paul ADENOT (:padenot) from comment #0)
> This changes the code path used after a seek from the normal: reader ->
> media queue -> output to use an intermediate step using MediaStreams, which
> is unnecessary.

It actually doesn't. The MediaStream it creates is simply not used.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 662352 [details] [diff] [review]
Patch

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

::: content/media/nsBuiltinDecoder.cpp
@@ +132,5 @@
>                       this, (long long)aStartTimeUSecs));
>  
>    DestroyDecodedStream();
>  
> +  if (!mOutputStreams.Length()) {

if (mOuputStreams.IsEmpty())

Comment 3

[Paul Adenot (:padenot)]

Attachment: Don't setup mediastreams in RecreateDecodedStream() if we did not have a mediastream before.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try since I don't see any results here. Will checkin if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=f9cf6f65e19f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Looks like this or bug 791344 is causing OSX crashtest failures.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15325977&tree=Try

TEST-UNEXPECTED-FAIL | file:///builds/slave/talos-slave/test/build/reftest/tests/content/media/test/crashtests/752784-1.html | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:01:23.503429
INFO | automation.py | Reading PID log: /var/folders/3t/f8_jgcpj1_s59lzd72ny3wyh00000w/T/tmputbtvjpidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9cf6f65e19f/try-macosx64-debug/firefox-18.0a1.en-US.mac64.crashreporter-symbols.zip
PROCESS-CRASH | file:///builds/slave/talos-slave/test/build/reftest/tests/content/media/test/crashtests/752784-1.html | application crashed (minidump found)
Crash dump filename: /var/folders/3t/f8_jgcpj1_s59lzd72ny3wyh00000w/T/tmpqjrRQd/minidumps/27D53FF3-B731-4410-B87B-63FB306229E4.dmp
Operating system: Mac OS X
                  10.8.0 12A269
CPU: amd64
     family 6 model 42 stepping 7
     8 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x40

Thread 0 (crashed)
 0  XUL!nsBuiltinDecoder::ConnectDecodedStreamToOutputStream [nsAutoPtr.h : 1003 + 0x0]
    rbx = 0x04b643f0   r12 = 0x19a6bb60   r13 = 0x19a6ba80   r14 = 0x19a6ba80
    r15 = 0x42334978   rip = 0x01cdb5f5   rsp = 0x5fbf95d0   rbp = 0x5fbf95f0
    Found by: given as instruction pointer in context
 1  XUL!nsBuiltinDecoder::AddOutputStream [nsBuiltinDecoder.cpp : 181 + 0xa]
    rip = 0x01cdc56c   rsp = 0x5fbf9600
    Found by: stack scanning
 2  XUL + 0xcdc45f
    rip = 0x01cdc460   rsp = 0x5fbf9610
    Found by: stack scanning
 3  XUL!nsHTMLMediaElement::CaptureStreamInternal [nsHTMLMediaElement.cpp : 1551 + 0x13]
    rip = 0x0188e542   rsp = 0x5fbf9640
    Found by: stack scanning
 4  XUL!nsHTMLMediaElement::MozCaptureStream [nsHTMLMediaElement.cpp : 1559 + 0x4]
    rip = 0x0188e590   rsp = 0x5fbf9680
    Found by: stack scanning
 5  XUL!NS_InvokeByIndex_P [xptcinvoke_x86_64_unix.cpp : 162 + 0x3]
    rip = 0x0274ebcf   rsp = 0x5fbf96a0
    Found by: stack scanning
 6  XUL!CallMethodHelper::ConvertIndependentParam [XPCWrappedNative.cpp : 2836 + 0x15]
    rip = 0x01f3bd38   rsp = 0x5fbf96b0
    Found by: stack scanning
 7  XUL!CallMethodHelper::InitializeDispatchParams [nsTArray.h : 192 + 0x0]
    rip = 0x01f3ac92   rsp = 0x5fbf9700
    Found by: stack scanning
 8  XUL!CallMethodHelper::Call [XPCWrappedNative.cpp : 3105 + 0x4]
    rip = 0x01f39e15   rsp = 0x5fbf9740
    Found by: stack scanning
 9  XUL!XPCWrappedNative::CallMethod [XPCWrappedNative.cpp : 2405 + 0x7]
    rip = 0x01f37b22   rsp = 0x5fbf9780
    Found by: stack scanning
10  XUL + 0xc1029f
    rip = 0x01c102a0   rsp = 0x5fbf97b8
    Found by: stack scanning

Thread 1
 0  libsystem_kernel.dylib + 0x12d16
    rbx = 0x04704648   r12 = 0x7317ff40   r13 = 0x7317ff88   r14 = 0x731809c0
    r15 = 0x73180934   rip = 0x8a972d16   rsp = 0x04704508   rbp = 0x04704680

Comment 6

[Paul Adenot (:padenot)]

Attachment: Don't setup mediastreams in RecreateDecodedStream() if we did not have a mediastream before.

Ha, I forgot to check all the callsites, it was even mentionned in the comment
in the header file.

Anyway, we have two solution to fix this, either changing the API a bit
(CreateDecodedStream and RecreateDecodedStream, perhaps), or checking if we
actually have a decoded stream when seeking, and avoid recreating one if it's
not needed. This implements the latter, but I have to admit I'm not too familiar
with the MediaStreams API, and I might be missing a use case that would make
this patch be wrong. For what I've tested, it seems to work.

Comment 7

[Paul Adenot (:padenot)]

Pushed to try, green: https://tbpl.mozilla.org/?tree=Try&rev=75d8ed08b09c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c500f57ba296

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c500f57ba296

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 662417 [details] [diff] [review]
Don't setup mediastreams in RecreateDecodedStream() if we did not have a mediastream before.

Requesting esr17 approval for this to fix bug 834667, which is a regression from bug 794426 which landed on esr17.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: massive memory growth after seeking in a video
User impact if declined: unusable browser or OOM crash after seeking a video
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): extremely low, avoids creating an unneeded resource, fix has baked on other branches for 4 months
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/7606a3baed96

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

I see that this was at one point marked "in testsuite?"

Should there be a test? Or is there a way for QA to verify? Thanks.

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to Matt Wobensmith from comment #12)
> I see that this was at one point marked "in testsuite?"
> 
> Should there be a test? Or is there a way for QA to verify? Thanks.

Matt can we try and verify https://bugzilla.mozilla.org/show_bug.cgi?id=834667 )steps in description) ? The patch uplifted in ths bug fixes the issue reported there and that could be a helpful testcase

Comment 14

[u279076]

I think this should have a testcase in-testsuite, but it's not clear to me if the steps to reproduce from bug 834667 can be converted into a testcase.

For the record:
1. enable HTML5 on http://youtube.com/html5
2. open this video https://www.youtube.com/watch?v=jTMxKUYrgJ4
3. skip onto 31:00
4. watch memory consumption grow very quickly