Avoid decoding 10 video frames up front for preload="metadata" videos

RESOLVED FIXED in mozilla31

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: wesj, Assigned: cpearce)

Tracking

(Blocks: 1 bug, {footprint, mobile, perf})

Trunk
mozilla31
footprint, mobile, perf
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 18 obsolete attachments)

299.47 KB, application/octet-stream
Details
14.32 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
HTML5 video elements can specify a preload attribute to hints whether or not the content should be loaded before the user clicks Play. This defaults to "metadata" on desktop to indicate that only enough should be load to get info about the video (i.e. length). In Fennec we seem to download the entire video anyway, resulting in excessive memory usage.

At the very least we should only be downloading the metadata for the video.
(Reporter)

Updated

7 years ago
tracking-fennec: --- → ?

Updated

7 years ago
tracking-fennec: ? → 2.0b5+

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 617852

Comment 2

7 years ago
This does not look like a dupe of that bug to me, we do not have nsSound in the stack trace at all. Here we get this:

--------------------------------------------------------------------------------
  n        time(i)         total(B)   useful-heap(B) extra-heap(B)    stacks(B)
--------------------------------------------------------------------------------
 48 14,426,715,483      113,630,124      112,701,023       929,101            0
99.18% (112,701,023B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc.
->47.99% (54,525,952B) 0x805AC87: huge_malloc (jemalloc.c:4694)
| ->46.14% (52,428,800B) 0x80582DD: imalloc (jemalloc.c:3864)
| | ->46.14% (52,428,800B) 0x805CC63: malloc (jemalloc.c:5878)
| |   ->36.91% (41,943,040B) 0x4059CAA: moz_xmalloc (mozalloc.cpp:98)
| |   | ->36.91% (41,943,040B) 0x5C079C2: mozilla::layers::BasicPlanarYCbCrImage::SetData(mozilla::layers::PlanarYCbCrImage::Data const&) (mozalloc.h:241)
| |   |   ->36.91% (41,943,040B) 0x4F3C386: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long long, long long, long long, VideoData::YCbCrBuffer const&, int, long long) (nsBuiltinDecoderReader.cpp:205)
| |   |     ->36.91% (41,943,040B) 0x4F61020: nsWebMReader::DecodeVideoFrame(int&, long long) (nsWebMReader.cpp:731)
| |   |       ->33.22% (37,748,736B) 0x4F3555D: nsBuiltinDecoderStateMachine::DecodeLoop() (nsBuiltinDecoderStateMachine.cpp:291)
| |   |       | ->33.22% (37,748,736B) 0x4F3BBE6: nsRunnableMethodImpl<void (nsBuiltinDecoderStateMachine::*)(), true>::Run() (nsThreadUtils.h:345)
| |   |       |   ->33.22% (37,748,736B) 0x5AC0A8F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:633)
| |   |       |     ->33.22% (37,748,736B) 0x5A49A23: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
| |   |       |       ->33.22% (37,748,736B) 0x5ABFA01: nsThread::ThreadFunc(void*) (nsThread.cpp:278)
| |   |       |         ->33.22% (37,748,736B) 0x698E689: _pt_root (ptthread.c:187)
| |   |       |           ->33.22% (37,748,736B) 0x403F96C: start_thread (pthread_create.c:300)
| |   |       |             ->33.22% (37,748,736B) 0x6BADA4C: clone (clone.S:130)
| |   |       |               
| |   |       ->03.69% (4,194,304B) 0x4F3D698: nsBuiltinDecoderReader::DecodeVideoFrame() (nsBuiltinDecoderReader.h:541)
| |   |         ->03.69% (4,194,304B) 0x4F3DA91: VideoData* nsBuiltinDecoderReader::DecodeToFirstData<VideoData>(int (nsBuiltinDecoderReader::*)(), MediaQueue<VideoData>&) (nsBuiltinDecoderReader.cpp:366)
| |   |           ->03.69% (4,194,304B) 0x4F3CD99: nsBuiltinDecoderReader::FindStartTime(long long, long long&) (nsBuiltinDecoderReader.cpp:328)
| |   |             ->03.69% (4,194,304B) 0x4F39FDC: nsBuiltinDecoderStateMachine::FindStartTime() (nsBuiltinDecoderStateMachine.cpp:1398)
| |   |               ->03.69% (4,194,304B) 0x4F380A0: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:947)
| |   |                 ->03.69% (4,194,304B) 0x5AC0A8F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:633)
| |   |                   ->03.69% (4,194,304B) 0x5A49A23: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
| |   |                     ->03.69% (4,194,304B) 0x5ABFA01: nsThread::ThreadFunc(void*) (nsThread.cpp:278)
| |   |                       ->03.69% (4,194,304B) 0x698E689: _pt_root (ptthread.c:187)
| |   |                         ->03.69% (4,194,304B) 0x403F96C: start_thread (pthread_create.c:300)
| |   |                           ->03.69% (4,194,304B) 0x6BADA4C: clone (clone.S:130)
| |   |                             
| |   ->09.23% (10,485,760B) 0x4F8433B: vpx_memalign (vpx_mem.c:132)
| |   | ->09.23% (10,485,760B) 0x4F8704B: vp8_yv12_alloc_frame_buffer (yv12config.c:72)
| |   |   ->07.38% (8,388,608B) 0x4F66B68: vp8_alloc_frame_buffers (alloccommon.c:71)
| |   |   | ->07.38% (8,388,608B) 0x4F76966: vp8_decode_frame (decodframe.c:622)
| |   |   |   ->07.38% (8,388,608B) 0x4F7D758: vp8dx_receive_compressed_data (onyxd_if.c:372)
| |   |   |     ->07.38% (8,388,608B) 0x4F82096: vp8_decode (vp8_dx_iface.c:424)
| |   |   |       ->07.38% (8,388,608B) 0x4F82D49: vpx_codec_decode (vpx_decoder.c:127)
| |   |   |         ->07.38% (8,388,608B) 0x4F60E06: nsWebMReader::DecodeVideoFrame(int&, long long) (nsWebMReader.cpp:690)
| |   |   |           ->07.38% (8,388,608B) 0x4F3D698: nsBuiltinDecoderReader::DecodeVideoFrame() (nsBuiltinDecoderReader.h:541)
| |   |   |             ->07.38% (8,388,608B) 0x4F3DA91: VideoData* nsBuiltinDecoderReader::DecodeToFirstData<VideoData>(int (nsBuiltinDecoderReader::*)(), MediaQueue<VideoData>&) (nsBuiltinDecoderReader.cpp:366)
| |   |   |               ->07.38% (8,388,608B) 0x4F3CD99: nsBuiltinDecoderReader::FindStartTime(long long, long long&) (nsBuiltinDecoderReader.cpp:328)
| |   |   |                 ->07.38% (8,388,608B) 0x4F39FDC: nsBuiltinDecoderStateMachine::FindStartTime() (nsBuiltinDecoderStateMachine.cpp:1398)
| |   |   |                   ->07.38% (8,388,608B) 0x4F380A0: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:947)
| |   |   |                     ->07.38% (8,388,608B) 0x5AC0A8F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:633)
| |   |   |                       ->07.38% (8,388,608B) 0x5A49A23: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
| |   |   |                         ->07.38% (8,388,608B) 0x5ABFA01: nsThread::ThreadFunc(void*) (nsThread.cpp:278)
| |   |   |                           ->07.38% (8,388,608B) 0x698E689: _pt_root (ptthread.c:187)
| |   |   |                             ->07.38% (8,388,608B) 0x403F96C: start_thread (pthread_create.c:300)
| |   |   |                               ->07.38% (8,388,608B) 0x6BADA4C: clone (clone.S:130)
| |   |   |                                 
| |   |   ->01.85% (2,097,152B) 0x4F66C5B: vp8_alloc_frame_buffers (alloccommon.c:94)
| |   |     ->01.85% (2,097,152B) 0x4F76966: vp8_decode_frame (decodframe.c:622)
| |   |       ->01.85% (2,097,152B) 0x4F7D758: vp8dx_receive_compressed_data (onyxd_if.c:372)
| |   |         ->01.85% (2,097,152B) 0x4F82096: vp8_decode (vp8_dx_iface.c:424)
| |   |           ->01.85% (2,097,152B) 0x4F82D49: vpx_codec_decode (vpx_decoder.c:127)
| |   |             ->01.85% (2,097,152B) 0x4F60E06: nsWebMReader::DecodeVideoFrame(int&, long long) (nsWebMReader.cpp:690)
| |   |               ->01.85% (2,097,152B) 0x4F3D698: nsBuiltinDecoderReader::DecodeVideoFrame() (nsBuiltinDecoderReader.h:541)
| |   |                 ->01.85% (2,097,152B) 0x4F3DA91: VideoData* nsBuiltinDecoderReader::DecodeToFirstData<VideoData>(int (nsBuiltinDecoderReader::*)(), MediaQueue<VideoData>&) (nsBuiltinDecoderReader.cpp:366)
| |   |                   ->01.85% (2,097,152B) 0x4F3CD99: nsBuiltinDecoderReader::FindStartTime(long long, long long&) (nsBuiltinDecoderReader.cpp:328)
| |   |                     ->01.85% (2,097,152B) 0x4F39FDC: nsBuiltinDecoderStateMachine::FindStartTime() (nsBuiltinDecoderStateMachine.cpp:1398)
| |   |                       ->01.85% (2,097,152B) 0x4F380A0: nsBuiltinDecoderStateMachine::Run() (nsBuiltinDecoderStateMachine.cpp:947)
| |   |                         ->01.85% (2,097,152B) 0x5AC0A8F: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:633)
| |   |                           ->01.85% (2,097,152B) 0x5A49A23: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
| |   |                             ->01.85% (2,097,152B) 0x5ABFA01: nsThread::ThreadFunc(void*) (nsThread.cpp:278)
| |   |                               ->01.85% (2,097,152B) 0x698E689: _pt_root (ptthread.c:187)
| |   |                                 ->01.85% (2,097,152B) 0x403F96C: start_thread (pthread_create.c:300)
| |   |                                   ->01.85% (2,097,152B) 0x6BADA4C: clone (clone.S:130)
| |   |                                     
| |   ->00.00% (0B) in 1+ places, all below ms_print's threshold (01.00%)
I agree with azakai that this isn't a dup.  In bug 617852 lots of space is mmap'd, but hardly any of it is actually touched.  This sounds completely different.

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

Comment 4

7 years ago
A little offtopic, but is there a way to see in that massif data whether the allocated space was actually touched?
(Reporter)

Updated

7 years ago
Duplicate of this bug: 630758

Comment 6

7 years ago
Some notes from irc:

<kinetik> azakai: okay, so when we load a video we decode and queue 10 frames of video, the main allocation is that
<kinetik> (1280 * 720 * 4 * 10)
<kinetik> for preload="metadata" videos perhaps we should only decode 1, since it's likely we'll need to buffer before playback anyway
<azakai> that calculates to 35MB, which sounds right
 maybe it makes sense to cache a # of frames that depends on their size
 so, cache up to X MB total, X is a pref
<kinetik> the intent of the frame queue is to avoid decode stuttering, so the larger the video, the more you're likely to need to avoid stuttering
<azakai> yes, I see. for fennec though we would need to not cache, or cache very very little...
 stuttering is better than running out of memory on a small phone ;)
Component: General → Video/Audio
Product: Fennec → Core
QA Contact: general → video.audio
Summary: Entire videos are automatically preloaded by Fennec → Avoid decoding 10 video frames up front for preload="metadata" videos
The intent of the preload="metadata" attribute is to avoid fetching data that may never be used.  I think it also makes sense to decode as little as possible up front to save runtime memory usage.  It's very likely that we'll need to enter buffering state and wait for more data from the network before playing back, so that gives us some time to fill the decode frame queue.
(Reporter)

Updated

7 years ago
Assignee: nobody → wjohnston
(In reply to comment #4)
> A little offtopic, but is there a way to see in that massif data whether the
> allocated space was actually touched?

Massif can't, but DHAT can: http://blog.mozilla.com/jseward/2010/12/05/fun-n-games-with-dhat/
(Assignee)

Comment 9

7 years ago
One way to implement this would be to expose nsHTMLMediaElement::mPreloadAction to the nsBuilinDecoderStateMachine, and in the DECODER_STATE_DECODING_METADATA case of nsBuilinDecoderStateMachine::Run(), don't call StartDecodeThreads() and  StartPlayback() if the media element has preload action PRELOAD_METADATA, and shut down the state machine thread (by dispatching a ShutdownThreadEvent) at the end of the DECODER_STATE_DECODING_METADATA case. You'd need to make access to the preload action thread safe.
One thing to watch out for is that FindStartTime() (called during DECODING_METADATA) can result in a bunch of video frames being decoded (for Ogg, at least).  With a random video I checked, there were 5 frames queued by the time FindStartTime returned.  We might not want to worry about this immediately, but for this fix to be effective for this case we'll need some logic to explicitly discard that decoded data (or avoid decoding it, just grab the timestamps we're looking for) and decode it again later when need.
(Assignee)

Comment 11

7 years ago
Ah yes, good point. To get the Ogg start time we need to at least demux up to the first Ogg packet with a granulepos, which can be a few packets. To discard the decoded data as Matthew suggests, you can call ResetDecode() on the nsBuiltinDecoderStateMachine::mReader at the end of the *_DECODING_METADATA case. You'd also need to ensure the nsOggReader's nsMediaStream was seeked to nsOggReader::mDataOffset, else the decode position would no longer be at the start of media.

Or reimplement nsOggReader::FindStartTime() and nsOggReader::DecodeVideoFrame() to not decode the video frames, but store them as demuxed ogg_packets awaiting decoding. The video frames would at least be smaller while stored as compressed ogg_packets.
(Reporter)

Comment 12

7 years ago
Created attachment 511074 [details] [diff] [review]
WIP

This is a WIP that implements most of what was mentioned above. Asking for feedback as I'm pretty new to this code. Also pushed this to try this morning to see what happens.

This reduces memory use to around 12MB. The remaining allocations seem to be about 5MB-8MB from nsTheoraState::Init() and another few MB from rendering the first frame (which for this 720P video is approximately 4MB).
Attachment #511074 - Flags: feedback?
(Assignee)

Comment 13

7 years ago
(In reply to comment #12)
> Created attachment 511074 [details] [diff] [review]
> This is a WIP that implements most of what was mentioned above. Asking for
> feedback as I'm pretty new to this code.

Thanks for working on this!

* In the DECODER_STATE_DECODING_METADATA case, you should only call ResetDecode() and re-seek the stream if the preload is != ENOUGH, otherwise you'll be throwing away data that we may actually need straight away.
* When you seek after calling ResetDecode(), don't seek to mDecoderPosition, that's the decoder's read offset after the decoder has read the first few frames (so if you start decoding there, it will miss the first few frames). You should seek to mReader->GetInfo().mDataOffset, that's the offset of the first media data (i.e. not header, metadata, container stuff, etc).
* In the DECODER_STATE_DECODING case, if you "continue" when we're in preloading metadata, you'll create a busy loop. Can you kill the state machine thread using a ShutdownThreadEvent, like we do at the end of the DECODER_STATE_COMPLETED case? We should automatically create a new thread if/when we start playback.
How much trouble would it be to also free the th_dec_ctx (presumably that's responsible for the 5-8 MB allocated from nsTheoraState::Init()), and re-create it again when you actually begin playback? There doesn't seem to be any real reason to keep it around after you've finished decoding the first frame, if you're not going to continue playback.
(Reporter)

Comment 15

7 years ago
Created attachment 511233 [details]
Memory Stack

Sorry... probably should have included this stack too.

I'm going to try killing the nsTheoraState when I can resetDecode. That should clear the setup info and the decoder info. Not sure what complications that will have when I try to play the stream.
(Reporter)

Comment 16

7 years ago
Created attachment 511253 [details] [diff] [review]
WIP v2

Adds changes listed. Still looking at ways to clean up some memory when ResetDecode is called.
Attachment #511074 - Attachment is obsolete: true
Attachment #511253 - Flags: feedback?
Attachment #511074 - Flags: feedback?
(Reporter)

Comment 17

7 years ago
Created attachment 511543 [details] [diff] [review]
Patch 1 - Move default to a pref

Arrghh. Crashing too much trying to throw away decoder allocated stuff.

I'm gonna put these up for review and maybe we can use follow ups for anything else. This patch allows us to control the default behavior here with a pref. That way we can set it to PRELOAD_NONE on mobile and use essentially zero bytes on videos that don't specify a preference. This doesn't conflict with my reading of the spec:

"The empty string is also a valid keyword, and maps to the Automatic state. The attribute's missing value default is user-agent defined, though the Metadata state is suggested as a compromise between reducing server load and providing an optimal user experience."
Attachment #511543 - Flags: review?(chris)
(Reporter)

Comment 18

7 years ago
Comment on attachment 511253 [details] [diff] [review]
WIP v2

Also putting this up for review, for cases where authors do specify that they want metadata loaded.

Just pushed both of these to try. Waiting to see how that goes as well.
Attachment #511253 - Flags: feedback? → review?(chris)
(Reporter)

Comment 19

7 years ago
Created attachment 511547 [details] [diff] [review]
Mobile Browser pref

Assuming we decide its ok to do this, we need to add this pref in mobile-browser.
(Assignee)

Comment 20

7 years ago
(In reply to comment #18)
> Comment on attachment 511253 [details] [diff] [review]
> WIP v2



>     case DECODER_STATE_DECODING:
>       {
>+        if (mDecoder->GetPreloadAction() == nsHTMLMediaElement::PRELOAD_METADATA) {
>+          // Shutdown the state machine thread, in order to save
>+          // memory on thread stacks, particuarly on Linux.
>+          nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(mDecoder->mStateMachineThread);
>+          NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
>+          mDecoder->mStateMachineThread = nsnull;
>+          return NS_OK;
>+        }
>+


The preload action can't change once the load has begun (because there's a guard on mBegun in UpdatePreloadAction()). If we have a non-playing preload=metadata element, we'll shutdown the state machine thread when the state machine reaches DECODING state, but when we start playback, the preload action doesn't change to reflect that we should download/decode the entire media. So the state machine thread will be shutdown again as soon as it enters the DECODING case again. I think this is causing your timeouts on TryServer.

We're better off not letting the state machine thread continue past DECODING_METADATA if preload!=enough, rather than trying to synchronize the preload action across threads. So instead of destroying the state machine thread in the DECODING case, destroy it in the DECODING_METADATA case. (I didn't realise shutting down the thread in the DECODING case would be a problem when I suggested it, sorry to lead you astray!) You should only destroy the thread (and call ResetDecode() and reseek the stream) if the preload action is != PRELOAD_ENOUGH and |mDecoder->GetState()==PLAY_STATE_PLAYING)|.

When made this change locally, the only failures left in content/media mochitests were in test_preload_action which you'll need to debug. Let me know if you get stuck.

(In reply to comment #19)
> Created attachment 511547 [details] [diff] [review]
> Mobile Browser pref
> 
> Assuming we decide its ok to do this, we need to add this pref in
> mobile-browser.

I imagine that some of the mochitests may rely on the default preload action being preload=metadata. So if you add a pref to change the default, you might get test failures when running the mochitests on Fennec (are we running the tests for Fennec? I saw a mochitest-1 result in your TryServer push...) Maybe we need to change the preload action pref metadata when running tests to ensure they run properly on Fennec. You could do that in manifest.js? I think all media mochitests include that. There's code to set prefs in use_large_case.js, you could use that as a guide to do this.
Keywords: footprint, mobile, perf
Whiteboard: [has-patch]
(Reporter)

Comment 21

7 years ago
Thanks for the detailed review. I have all of the tests in content/media passing locally now. Running through the rest of mochitest, then I'll upload them.

I am curious what sort of behavior we want on mobile when a page sends:

<video preload src="something"/>
or
<video preload="auto" src="something"/>

The spec says that both of those should do the same thing, and that "the user agent can put the user's needs first without risk to the server, up to and including optimistically downloading the entire resource."

I feel within our rights to download nothing according to the spec, but I don't know that web authors would anticipate that. Are there strong opinions on the subject?
(Reporter)

Comment 22

7 years ago
Created attachment 511864 [details] [diff] [review]
Reduce Metadata Patch v3
Attachment #511253 - Attachment is obsolete: true
Attachment #511864 - Flags: review?(chris)
Attachment #511253 - Flags: review?(chris)
(Reporter)

Comment 23

7 years ago
Created attachment 511867 [details] [diff] [review]
Change defaults Patch v2

Adds two prefs now: mobile.preload.default and mobile.preload.auto Just pushed these to try again:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=2c1018045d04

content/media tests pass locally. I also added some code to check for these prefs in tests, although I don't think these particular tests run in Fennec currently. Likely it will not change much.

I also saw videocontrols tests fail on two builds on try, but I can't seem to reproduce that locally either (at least, not with these changes).
Attachment #511543 - Attachment is obsolete: true
Attachment #511867 - Flags: review?(chris)
Attachment #511543 - Flags: review?(chris)
(Reporter)

Comment 24

7 years ago
Arrgh. more try server errors. Will look at in a bit...
(In reply to comment #23)
> Created attachment 511867 [details] [diff] [review]
> Change defaults Patch v2
> 
> Adds two prefs now: mobile.preload.default and mobile.preload.auto Just pushed

You mean "media.preload.default" and "media.preload.auto"
(Reporter)

Comment 26

7 years ago
Created attachment 512003 [details] [diff] [review]
Reduce Metadata Patch v4

Ahh. I assumed it was a typo to check if:

mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING

as well as checking the preload action. Doing this the way you suggested things are looking much better on try:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=0a24c11b9e23
Attachment #511864 - Attachment is obsolete: true
Attachment #512003 - Flags: review?(chris)
Attachment #511864 - Flags: review?(chris)
(Assignee)

Comment 27

7 years ago
media.preload.auto doesn't do anything, it stores and sets a value which was already set. If something is preload=auto, we still want to load that video as if it's preload=auto, don't we? Why do we need to have this pref? Probably should just remove it.

If we're playing, we must not stop the decode thread, even if the preload action is != ENOUGH. If we're playing, we must load and decode the media so we can play it. Playing trumps preload!

>Bug 631058 - Part 1
>
>diff --git a/content/media/nsBuiltinDecoderStateMachine.cpp b/content/media/nsBuiltinDecoderStateMachine.cpp
>--- a/content/media/nsBuiltinDecoderStateMachine.cpp
>+++ b/content/media/nsBuiltinDecoderStateMachine.cpp
>@@ -982,21 +982,22 @@ nsresult nsBuiltinDecoderStateMachine::R
>           continue;
>         }
> 
>         VideoData* videoData = FindStartTime();
>         if (videoData) {
>           MonitorAutoExit exitMon(mDecoder->GetMonitor());
>           RenderVideoFrame(videoData);
>         }
>-
>-        // Start the decode threads, so that we can pre buffer the streams.
>-        // and calculate the start time in order to determine the duration.
>-        if (NS_FAILED(StartDecodeThreads())) {
>-          continue;
>+        if (mDecoder->GetPreloadAction() == nsHTMLMediaElement::PRELOAD_ENOUGH) {
>+          // Start the decode threads, so that we can pre buffer the streams.
>+          // and calculate the start time in order to determine the duration.
>+          if (NS_FAILED(StartDecodeThreads())) {
>+            continue;
>+          }
>         }

If we're playing we need the decode thread running, so this condition should be:

    if (mDecoder->GetPreloadAction() == nsHTMLMediaElement::PRELOAD_ENOUGH ||
        mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING)
    {
      /* ... */
    }

    
>@@ -1025,16 +1026,32 @@ nsresult nsBuiltinDecoderStateMachine::R
>           mState = DECODER_STATE_DECODING;
>         }
> 
>         // Start playback.
>         if (mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING) {
>           if (!IsPlaying()) {
>             StartPlayback();
>           }
>+          if (mDecoder->GetPreloadAction() != nsHTMLMediaElement::PRELOAD_ENOUGH) {
>+            nsMediaStream* stream = mDecoder->GetCurrentStream();
>+            if (mReader) {
>+              MonitorAutoExit exitMon(mDecoder->GetMonitor());
>+              mReader->ResetDecode();
>+              stream->Seek(nsISeekableStream::NS_SEEK_SET, mReader->GetInfo().mDataOffset);
>+            }
>+
>+            // Shutdown the state machine thread, in order to save
>+            // memory on thread stacks, particuarly on Linux.
>+            nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(mDecoder->mStateMachineThread);
>+            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
>+            mDecoder->mStateMachineThread = nsnull;
>+
>+            return NS_OK;
>+          }
>         }
>       }
>       break;

StartPlayback() releases the decoder monitor, so mState can change state when calling that. Also when you release the monitor to call ResetDecode() the decoder can change state (nsAudioMetadataEventRunner dispatches a "loadedmetadata" event, the JS handler can cause a state change).

If we changed to SHUTDOWN or SEEKING state while loading metadata, we'll still kill the thread with this change, but we need the state thread alive to action those states; otherwise the state machine would get stuck in those states. We also don't want to stop decoding if we're playing - we need data to play! This change will still destroy the state machine thread if we're playing, but have preload=metadata. So you need to reverify the state after releasing the decoder monitor.

So we should only stop the decode thread if we're *not* playing, and the state machine is in DECODING state, and we don't have preloadAction=ENOUGH.

I think we should just merge this block with |if (mState == METADATA)| block before it (since there's no reason to start playback if we've moved to a non-decoding state anyway) so just replace those two blocks with:

        if (mState == DECODER_STATE_DECODING_METADATA) {
          LOG(PR_LOG_DEBUG, ("%p Changed state from DECODING_METADATA to DECODING", mDecoder));
          mState = DECODER_STATE_DECODING;

          // Start playback.
          if (mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING) {
            if (!IsPlaying()) {
              StartPlayback();
            }
          } else if (mDecoder->GetPreloadAction() != nsHTMLMediaElement::PRELOAD_ENOUGH) {
            nsMediaStream* stream = mDecoder->GetCurrentStream();
            if (mReader) {
              MonitorAutoExit exitMon(mDecoder->GetMonitor());
              mReader->ResetDecode();
              stream->Seek(nsISeekableStream::NS_SEEK_SET, mReader->GetInfo().mDataOffset);
            }

            // Note state can change when we release the decoder monitor to
            // call ResetDecode() above, so we must re-verify the state here.
            if (mState != DECODER_STATE_DECODING ||
                mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING)
            {
              continue;
            }

            // Shutdown the state machine thread, in order to save
            // memory on thread stacks, particuarly on Linux.
            nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(mDecoder->mStateMachineThread);
            NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
            mDecoder->mStateMachineThread = nsnull;
            
            return NS_OK;
          }
        }
        

Can you also add comments saying why we call ResetDecode()?

Don't create use_preload.js. Put the code to set media.preload.default in manifest.js. That file is included in every media test. Then we don't have to worry when writing new tests about whether we need to include another JS file in order to get the test to pass on mobile. We also won't be left wondering why newly added tests time out on TryServer on mobile but not on other platforms (because we forgot to include that extra JS file).
(Reporter)

Comment 28

7 years ago
I've got these done. Am running tests locally and then will upload/push to try again. Wanted to ask/answer a few questions. 

> media.preload.auto doesn't do anything, it stores and sets a value which was
> already set. If something is preload=auto, we still want to load that video as
> if it's preload=auto, don't we? Why do we need to have this pref? Probably
> should just remove it.

My thinking is that "auto" gives us freedom to choose what we think is best. In Firefox, "auto"="enough" (which isn't really an HTML5 preload value, but we can do whatever we want). In Fennec I'm proposing that when we see "auto" we choose "none".
 
> So we should only stop the decode thread if we're *not* playing, and the state
> machine is in DECODING state, and we don't have preloadAction=ENOUGH.

Aha! I wasn't crazy! Just didn't know about the state changing. Just out of curiosity, sounds like we could have just left this in the DECODER_STATE_DECODING area with an additional IsPlaying() check? Thanks again for the help.

> Can you also add comments saying why we call ResetDecode()?

Done.

>  Put the code to set media.preload.default in manifest.js. That file is included in every media test.

Hmm... I did this, but wanted to note that not all of the media tests actually seem to import manifest.js, including test_preload_attribute.html and test_preload_suspend.html. Maybe they are including it in some way I can't see... ?
(Reporter)

Comment 29

7 years ago
Hmm... am getting the same error in test_mozfiledataurl.html. I think that the canplay event still isn't firing with this change....
(Reporter)

Comment 30

7 years ago
Created attachment 512186 [details] [diff] [review]
Reduce Metadata Patch v4.1

Just updating for anyone playing along. Trying to dig backwards from canplay events to see if there is some way to kill the thread and still fire the event.
(Reporter)

Comment 31

7 years ago
Created attachment 512187 [details] [diff] [review]
Change defaults Patch v3

Moves stuff into manifest.js, but keeps my media.preload.auto pref around until I get an answer.

This patch alone should actually fix a lot of our problems on mobile, without worrying about the metadata stuff.
Attachment #511867 - Attachment is obsolete: true
Attachment #512003 - Attachment is obsolete: true
Attachment #512187 - Flags: review?(chris)
Attachment #511867 - Flags: review?(chris)
Attachment #512003 - Flags: review?(chris)
(Reporter)

Comment 32

7 years ago
So the problem here seems to be that we are throwing out all of our "extra" frames and also throwing away the decoder. The decoder pushes our current frame status to the element:

http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoder.cpp#638

the element looks at it, and if we say "NEXT_FRAME_AVAILABLE", sends a canplay event:

http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1481

Since we are throwing away the decoder now though, the canplay event isn't firing.I don't want to change behavior, but I'm not exactly sure what the right thing to do here is. I can fix the test by just adding a preload attribute on the audio element here:

http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_mozfiledataurl.html?force=1#15
(Reporter)

Comment 33

7 years ago
Alternatively, we could make it so that <video preload> and <video> do the same thing:
  "enough" on Firefox and
  "none" on Fennec.
Looking right now to see what the current difference is between "enough" and "metadata" (before these patches) anyway. If there isn't much, there won't be much change except for sites specifying <video preload="metadata">
(Assignee)

Comment 34

7 years ago
Comment on attachment 512187 [details] [diff] [review]
Change defaults Patch v3

>Bug 631058 - Part 2
>
>diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
>--- a/content/html/content/src/nsHTMLMediaElement.cpp
>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
>@@ -841,34 +841,38 @@ void nsHTMLMediaElement::UpdatePreloadAc
>   if ((IsAutoplayEnabled() && HasAttr(kNameSpaceID_None, nsGkAtoms::autoplay)) ||
>       !mPaused)
>   {
>     nextAction = nsHTMLMediaElement::PRELOAD_ENOUGH;
>   } else {
>     // Find the appropriate preload action by looking at the attribute.
>     const nsAttrValue* val = mAttrsAndChildren.GetAttr(nsGkAtoms::preload,
>                                                        kNameSpaceID_None);
>+    PRUint32 preloadDefault = nsContentUtils::GetIntPref("media.preload.default",
>+                            nsHTMLMediaElement::PRELOAD_ATTR_METADATA);
>+    PRUint32 preloadAuto = nsContentUtils::GetIntPref("media.preload.auto",
>+                            nsHTMLMediaElement::PRELOAD_ENOUGH);
>     if (!val) {
>-      // Attribute is not set. The default is to load metadata.
>-      nextAction = nsHTMLMediaElement::PRELOAD_METADATA;
>+      // Attribute is not set. Use the pref

The comment is not terribly descriptive, can you change that too:

// Attribute is not set. Use the preload action specified by the 
// media.preload.default pref, or just preload metadata if not present.

>+      nextAction = static_cast<PreloadAction>(preloadDefault);
>     } else if (val->Type() == nsAttrValue::eEnum) {
>       PreloadAttrValue attr = static_cast<PreloadAttrValue>(val->GetEnumValue());
>       if (attr == nsHTMLMediaElement::PRELOAD_ATTR_EMPTY ||
>           attr == nsHTMLMediaElement::PRELOAD_ATTR_AUTO)
>       {
>-        nextAction = nsHTMLMediaElement::PRELOAD_ENOUGH;
>+        nextAction = static_cast<PreloadAction>(preloadAuto);
>       } else if (attr == nsHTMLMediaElement::PRELOAD_ATTR_METADATA) {
>         nextAction = nsHTMLMediaElement::PRELOAD_METADATA;
>       } else if (attr == nsHTMLMediaElement::PRELOAD_ATTR_NONE) {
>         nextAction = nsHTMLMediaElement::PRELOAD_NONE;
>       }
>     } else {
>       // There was a value, but it wasn't an enumerated value.
>-      // Use the suggested "missing value default" of "metadata".
>-      nextAction = nsHTMLMediaElement::PRELOAD_METADATA;
>+      // Use the pref

Can you change the comment to:

// Use the suggested "missing value default" of "metadata", or the value
// specified by the media.preload.default, if present.

I'd recommend you use the value for preload=metadata for media.preload.auto on mobile. It's not a huge bandwidth cost, and means you'll get the media's first frame displayed at the correct dimensions, rather than a grey box. It's you mobile guys' call of course. ;)
Attachment #512187 - Flags: review?(chris) → review+
(Assignee)

Comment 35

7 years ago
Looks good!

(In reply to comment #31)
> Created attachment 512187 [details] [diff] [review]
> Change defaults Patch v3
> 
> Moves stuff into manifest.js, but keeps my media.preload.auto pref around until
> I get an answer.

Alright, seems reasonable to keep that pref.

(In reply to comment #28)
> 
> > So we should only stop the decode thread if we're *not* playing, and the state
> > machine is in DECODING state, and we don't have preloadAction=ENOUGH.
> 
> Aha! I wasn't crazy! Just didn't know about the state changing. Just out of
> curiosity, sounds like we could have just left this in the
> DECODER_STATE_DECODING area with an additional IsPlaying() check? Thanks again
> for the help.

Because the preloadAction doesn't change during a load, if we destroyed the state machine thread in the DECODING case, we'd destroy the decoder thread and call ResetDecode() and throw away buffered frames etc everytime a load which started as preload=metadata (but is playing, so is loading fully now) was paused, which is not ideal. 

> 
> > Can you also add comments saying why we call ResetDecode()?
> 
> Done.

Full stops at the end of sentences in comments please!
"// to the start of the stream" -> "// to the start of the stream to save memory, particularly on mobile."

Can you move your comment "// Start the decode threads, so that we can pre buffer the streams..." down so it starts on its own line, rather than on the same line as the preceding "{".

> 
> >  Put the code to set media.preload.default in manifest.js. That file is included in every media test.
> 
> Hmm... I did this, but wanted to note that not all of the media tests actually
> seem to import manifest.js, including test_preload_attribute.html and
> test_preload_suspend.html. Maybe they are including it in some way I can't
> see... ?

Yeah, only the tests which use our MediaTestManager, or use one of the resources in the manifest need include manifest.js. Most of them do... We're trying to make all the tests which dynamically load resource use the MediaTestManager where possible, as then we can control the level of parallelism of the tests with a single parameter.


(In reply to comment #32)
[...]
> Since we are throwing away the decoder now though, the canplay event isn't
> firing.I don't want to change behavior, but I'm not exactly sure what the right
> thing to do here is. I can fix the test by just adding a preload attribute on
> the audio element here:
> 
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_mozfiledataurl.html?force=1#15

We shouldn't fire canplay if we don't have future data to play, so we shouldn't fire canplay in this case. Can you listen on loadedmetadata instead?
(Reporter)

Comment 36

7 years ago
Created attachment 512295 [details] [diff] [review]
Reduce Metadata Patch v5

For checkin (not sure if this is + yet or not). Everything is happy locally. Going to push through try one more time.
Attachment #512186 - Attachment is obsolete: true
Attachment #512295 - Flags: review?(chris)
(Reporter)

Comment 37

7 years ago
Created attachment 512296 [details] [diff] [review]
Change defaults Patch v4

For checkin
Attachment #512187 - Attachment is obsolete: true
Attachment #512296 - Flags: review+
(Reporter)

Comment 38

7 years ago
Created attachment 512299 [details] [diff] [review]
Mobile Browser prefs

Prefs for mobile-browser.
Attachment #511547 - Attachment is obsolete: true
Attachment #512299 - Flags: review?(mark.finkle)
(Assignee)

Comment 39

7 years ago
Comment on attachment 512295 [details] [diff] [review]
Reduce Metadata Patch v5

Looks good!
Attachment #512295 - Flags: review?(chris) → review+
Comment on attachment 512299 [details] [diff] [review]
Mobile Browser prefs


> // prevent tooltips from showing up
> pref("browser.chrome.toolbar_tips", false);
> pref("indexedDB.feature.enabled", false);
>+pref("media.preload.default", 1); // default to preload none
>+pref("media.preload.auto", 2);    // preload metadata if preload=auto

Add a line break and a short comment here, telling us what this does.
Attachment #512299 - Flags: review?(mark.finkle) → review+
(Reporter)

Comment 41

7 years ago
Created attachment 512347 [details] [diff] [review]
Mobile Browser prefs for checkin

For chekcin
Attachment #512299 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/2e49069ac1ad
http://hg.mozilla.org/mozilla-central/rev/a8d213604ca6

http://hg.mozilla.org/mobile-browser/rev/ba2c0856bc3c
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297745230.1297745561.23507.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297745963.1297747046.29487.gz

and maybe

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297745229.1297746243.26496.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
cpearce, wesj - any idea about those failures/crash?
(Reporter)

Comment 45

7 years ago
Commenting out:

mDecoder->mStateMachineThread = nsnull;

fixes this locally for me.
push to try plz
(Reporter)

Comment 47

7 years ago
already running: http://tbpl.mozilla.org/?tree=MozillaTry&rev=2375e461c608
(Assignee)

Comment 48

7 years ago
All the orange boxes on tbpl are starred? The boxes which failed on the push are commented with "should be fixed by backout of 2c646d10b9c7". The tree also looks greenish after the checkin, and those tests which were listed as failing don't have any video or audio elements in them. Is there actually a problem here?
(Reporter)

Comment 49

7 years ago
We backed this patch out, which fixed the webgl tests. One of the webgl tests involves using a video element as a source for a texture. The other failures were unrelated I think.
(Reporter)

Comment 50

7 years ago
Doing this resulted in test failures in content/media that I can't reproduce locally (yet). I also can't play videos with preload="metadata" locally when I do this either though, so I'm looking for a better solution.
(Assignee)

Comment 51

7 years ago
The backout and merge DougT performed:

http://hg.mozilla.org/mozilla-central/rev/8cc61e7d5f8d
http://hg.mozilla.org/mozilla-central/rev/4729f3e52c39
http://hg.mozilla.org/mozilla-central/rev/2c646d10b9c7

It looks like the problem is that nsWebMReader initializes its mDataOffset field to -1, so when we call |stream->Seek(nsISeekableStream::NS_SEEK_SET, mReader->GetInfo().mDataOffset)| for WebM, we'll start getting assertion failures in the nsMediaCache read since it's been seeked to offset -1.

We should probably initialize nsWebMReader::mInfo.mDataOffset to 0 rather than -1 in nsWebMReader::ReadMetadata(). Put this change in its own patch and request review from Matthew Gregan, since he has a much better understanding of how this change would affect the WebM parser than I.

We should also:
* Check for failure of the Seek(info.mDataOffset) call in DECODING_METADATA, and move to SHUTDOWN state if the seek fails (sorry I overlooked that in my review!).
* Check in nsMediaCacheStream::Seek() whether the seek target (mStreamOffset) will be < 0, and return failure if so.
(Reporter)

Comment 52

7 years ago
Created attachment 512661 [details] [diff] [review]
Reduce Metadata Patch v6

Thanks for the debugging help. This implements what you have mentioned, but will fail tests because all WebM videos with preload="metadata" will be forced into the DECODER_SHUTDOWN state. One line WebM patch coming.

On try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=907e3c4fdf1a
Attachment #512661 - Flags: review?(chris)
(Reporter)

Comment 53

7 years ago
Created attachment 512663 [details] [diff] [review]
WebM Fix v1

WebM patch
Attachment #512663 - Flags: review?
(Reporter)

Updated

7 years ago
Attachment #512663 - Flags: review? → review?(kinetik)
Comment on attachment 512663 [details] [diff] [review]
WebM Fix v1

This should be fine.  It'd be best to remove the comment, since we're now relying on the value.
Attachment #512663 - Flags: review?(kinetik) → review+
(Assignee)

Comment 55

7 years ago
Comment on attachment 512661 [details] [diff] [review]
Reduce Metadata Patch v6

>@@ -1018,22 +1021,51 @@ nsresult nsBuiltinDecoderStateMachine::R
>         if (HasAudio()) {
>           mEventManager.Init(info.mAudioChannels, info.mAudioRate);
>           mDecoder->RequestFrameBufferLength(frameBufferLength);
>         }
> 
>         if (mState == DECODER_STATE_DECODING_METADATA) {
>           LOG(PR_LOG_DEBUG, ("%p Changed state from DECODING_METADATA to DECODING", mDecoder));
>           mState = DECODER_STATE_DECODING;
>-        }
> 
>-        // Start playback.
>-        if (mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING) {
>-          if (!IsPlaying()) {
>-            StartPlayback();
>+          // Start playback.
>+          if (mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING) {
>+            if (!IsPlaying()) {
>+              StartPlayback();
>+            }
>+          } else if (mDecoder->GetPreloadAction() != nsHTMLMediaElement::PRELOAD_ENOUGH) {
>+            if (mReader) {
>+              // Clear any frames queued up during FindStartTime() and reset
>+              // to the start of the stream  to save memory.
>+              MonitorAutoExit exitMon(mDecoder->GetMonitor());
>+              mReader->ResetDecode();
>+
>+              nsMediaStream* stream = mDecoder->GetCurrentStream();
>+              PRInt64 offset = mReader->GetInfo().mDataOffset;
>+              if (NS_FAILED(stream->Seek(nsISeekableStream::NS_SEEK_SET, offset)))
>+              {
>+                mState = DECODER_STATE_SHUTDOWN;
>+              }

For a single line "{" can you put the "{" at the end of the line?


>diff --git a/content/media/nsMediaCache.cpp b/content/media/nsMediaCache.cpp
>--- a/content/media/nsMediaCache.cpp
>+++ b/content/media/nsMediaCache.cpp
>@@ -2079,16 +2079,19 @@ nsMediaCacheStream::Seek(PRInt32 aWhence
>   case PR_SEEK_SET:
>     mStreamOffset = aOffset;
>     break;
>   default:
>     NS_ERROR("Unknown whence");
>     return NS_ERROR_FAILURE;
>   }
> 
>+  if (mStreamOffset < 0)
>+    return NS_ERROR_FAILURE;
>+
>   LOG(PR_LOG_DEBUG, ("Stream %p Seek to %lld", this, (long long)mStreamOffset));
>   gMediaCache->NoteSeek(this, oldOffset);
> 
>   gMediaCache->QueueUpdate();
>   return NS_OK;
> }
> 
> PRInt64

Can you change this so that mStreamOffset isn't changed on failure? e.g. have the switch statement operate on a copy of mStreamOffset, and assign that to mStreamOffset if it's >=0. 

Thanks!
(Reporter)

Comment 56

7 years ago
Created attachment 512672 [details] [diff] [review]
Reduce Metadata Patch v6.1
Attachment #512295 - Attachment is obsolete: true
Attachment #512661 - Attachment is obsolete: true
Attachment #512672 - Flags: review?(chris)
Attachment #512661 - Flags: review?(chris)
(Assignee)

Comment 57

7 years ago
Comment on attachment 512672 [details] [diff] [review]
Reduce Metadata Patch v6.1

Great, thanks very much!
Attachment #512672 - Flags: review?(chris) → review+
Passed try.

http://hg.mozilla.org/mozilla-central/rev/0000b936e5c6
http://hg.mozilla.org/mozilla-central/rev/60d7b8a4c275
http://hg.mozilla.org/mozilla-central/rev/2869e73e38e7
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
replaced 0000b936e5c6 with:

http://hg.mozilla.org/mozilla-central/rev/0d43e33ce134
(Assignee)

Updated

7 years ago
Depends on: 634532

Updated

7 years ago
Depends on: 634825

Comment 60

7 years ago
These are some pretty big changes to the decoder late in the game, with only one beta left. Would it be worth consider a smaller change (maybe not stopping the decoder thread?) and put off until post FF 4 the rest?
(Reporter)

Comment 61

7 years ago
I would be fine with that. In fact, like I mentioned earlier, the prefs patch fixes our problem on Fennec. Landing gave us enough data about the errors to write up a better patch, but I'd still like to wait and land it at a time when we can risk some crashiness.
Backing out changesets 0d43e33ce134 and 60d7b8a4c275.  We agree with Chris - lets do this after 4.0 ships.

http://hg.mozilla.org/mozilla-central/rev/1c61363cc39f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: 2.0b5+ → ---
Seems like this caused a topcrash at signature:
nsBuiltinDecoderStateMachine::Run()

https://crash-stats.mozilla.com/report/list?product=Firefox&branch=2.0&query_search=signature&query_type=exact&query=nsBuiltinDecoderStateMachine%3A%3ARun%28%29&date=02%2F28%2F2011%2023%3A59%3A59&range_value=4&range_unit=weeks&hang_type=crash&process_type=browser&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&admin=&signature=nsBuiltinDecoderStateMachine%3A%3ARun%28%29

Updated

7 years ago
Depends on: 634866
Depends on: 635703
Depends on: 635400
(Reporter)

Comment 64

7 years ago
Created attachment 520298 [details] [diff] [review]
WIP v7

This incorporates the change described by kinetic (I think?) to use mReader->Seek, which calls ResetDecode for us. It also gives up attempting to kill the decoder thread. Instead, I just put some code inside nsBuiltinDecoderStateMachine::DecodeLoop to only decode one frame (I think maybe I need to update that so that currentVideoPumpThreshold=2) if the video is not playing and the preload state is set to metadata.

I also ran into test errors because calling Seek() ends up calling back into UpdatePlaybackPositionInternal which can set the end time to the new time passed in, and change the reported duration of the video to zero (some tests expect video.duration = "NAN"). I'm a bit nervous that will interact badly with streaming video, so just asking for feedback on these bits for now, and to find out if there is a smarter way to handle this.
Attachment #512663 - Attachment is obsolete: true
Attachment #512672 - Attachment is obsolete: true
Attachment #520298 - Flags: feedback?(chris)
(Assignee)

Comment 65

7 years ago
Comment on attachment 520298 [details] [diff] [review]
WIP v7

(In reply to comment #64)
> Created attachment 520298 [details] [diff] [review]
> WIP v7

Looking at the top crash reported in comment 23, perhaps you need to check in nsBuiltinDecoder::GetPreloadAction() that mElement is non-null? Everywhere nsBuiltinDecoder uses mElement we verify it's non-null first, so not doing that may be the cause of the crash. It's probably safe to return PRELOAD_METADATA if !mElement, but you'll need to test that.

> This incorporates the change described by kinetic (I think?) to use
> mReader->Seek, which calls ResetDecode for us. It also gives up attempting to
> kill the decoder thread. Instead, I just put some code inside
> nsBuiltinDecoderStateMachine::DecodeLoop to only decode one frame (I think
> maybe I need to update that so that currentVideoPumpThreshold=2) if the video
> is not playing and the preload state is set to metadata.

Because the preload action doesn't change during the lifetime of a load, and also because you only initialize currentVideoPumpThreshold at the start of the decode thread run function, this will mean that we only ever decode ahead 1 frame while playing a preload=metadata video (the default behaviour on desktop). We need the decode-ahead so that if we have a long running video decode, we have frames to play in the meantime. Don't do this!
 
> I also ran into test errors because calling Seek() ends up calling back into
> UpdatePlaybackPositionInternal

I'm not sure why nsOggReader::Seek() calls UpdatePlaybackPosition(), I don't think it should be necessary (any more?). What happens if you remove the UpdatePlaybackPosition() call from nsOggReader::Seek()?

Why do you add a check inside ns*Reader::CanDecodeToTarget() that aTarget >= 0? It should always be >=0 anyway, did you encounter it being less than 0?

I think the only changes required to make your previous patch work, is to use Seek(mStartTime,...) instead of stream->Seem(mDataOffset...) (as you're doing), add the null check to nsBuiltinDecoder::GetPreloadAction(), and remove the UpdatePlaybackPosition() call from nsOggReader::Seek()?
Attachment #520298 - Flags: feedback?(chris) → feedback-
(Assignee)

Comment 66

7 years ago
I'm starting work on bug 592833, and that's going to change the threading model of the nsBuiltinDecoderStateMachine a bit, including moving decoding metadata over to the decode thread, and merging all state machine threads into one. So probably the best thing to do would be for you to produce a patch which doesn't change the creation/destruction of threads, and just (basically) calls ResetDecode() when loading metadata. Otherwise, I'll end up just having to reverse your threading changes in my patches for bug 592833, and we'll be wasting your efforts.
(Reporter)

Comment 67

7 years ago
> I'm not sure why nsOggReader::Seek() calls UpdatePlaybackPosition(), I don't
> think it should be necessary (any more?). What happens if you remove the
> UpdatePlaybackPosition() call from nsOggReader::Seek()?

Seems to work fine without. Thanks.

> Why do you add a check inside ns*Reader::CanDecodeToTarget() that aTarget >= 0?

I assume you mean aCurrentTime >= 0? I stole this from Kinetic in Bug 634532. We are passing in a -1 for aCurrentTime when we call ns*Reader::Seek in nsBuiltinDecoderStateMachine. I assume that we aren't exactly sure what aTarget will be (we are passing in mStartTime which is likely zero?), and we need to ensure that CanDecodeToTarget returns false (which it won't without this check since if aTarget=0).

In nsWebMReader, canDecodeToTarget returns false, so we reset the decoder and seek the nestegg to the correct position.

In the nsOggReader we actually shouldn't hit that check, but instead check that aTarget = aStartTime, which seeks the decoder, resets the decode, and the hits the UpdatePlaybackPosition call removed above. I can remove the check there, but it seems nice to have some symmetry between the two.

> Looking at the top crash reported in comment 23, perhaps you need to check in
> nsBuiltinDecoder::GetPreloadAction() that mElement is non-null? Everywhere
> nsBuiltinDecoder uses mElement we verify it's non-null first, so not doing that may be the cause of the crash. It's probably safe to return PRELOAD_METADATA if !mElement, but you'll need to test that.

I am a bit confused about that crash report, but it looks like you're right. I think I just remembered comment #1 in bug 634825. That implied it was the result of setting mDecoder->mStateMachineThread to null, which I have worried about before.

I've added the mElement check, and am trying to not shutdown the decoder thread here, which it sounds like you would also agree with. Unfortunately, at this point nsBuiltinDecoderStatemachine::mState is set to DECODER_STATE_DECODING. Talked about this on IRC:

wesj: hey! thanks for the feedback. i'm trying to not kill the decoder thread in that bug, but that causes the DecoderStateMachine to move into DECODER_STATE_DECODING.
wesj: cpearce: my trick to change AMPLE_VIDEO_FRAMES was my hack around that, but i wondered if there is a more correct one
cpearce: wesj: right, so we're moving to doing the metadata decoding on the decoder thread anyway, so you're better off not touching that code for the time being.
(02:01:40 PM) cpearce: wesj: cos we really don't want to change AMPLE_VIDEO_FRAMES
(02:03:59 PM) wesj: cpearce: hmm... so you'd be fine if I just let it run into the decoding state, eh?
(02:05:56 PM) cpearce: wesj: IIRC we tried that but had problems. I think you'd need to go into DECODING state, and in DECODING state you'd need to call ResetDecode();Seek(0) if the media had never been played before, and the element has preload action metadata.
(02:06:41 PM) cpearce: wesj: so you'd need to add tracking to detect if the media had ever been played to implement that.
(02:08:21 PM) cpearce: wesj: hmm, just thinking about it now, you're probably best to wait until I finish the dethreadification, as there's a better way to implement this in the new threading model. that happens in bug 592833.
(02:09:02 PM) cpearce: wesj: If you go implementing it now, we'd have to reverse your work anyway once the threading stuff landed, which would be wasting your efforts.
No longer depends on: 635400
(Assignee)

Updated

7 years ago
Depends on: 650994
(Reporter)

Comment 68

4 years ago
Unassigning things I'm not working on anymore.
Assignee: wjohnston → nobody
(Assignee)

Updated

4 years ago
Blocks: 982695
(Assignee)

Comment 69

4 years ago
I'll work on this in order to help fix bug 982695.
Assignee: nobody → cpearce
(Assignee)

Comment 70

4 years ago
Created attachment 8397428 [details] [diff] [review]
Patch2 v1: Reduce preroll for preload=metadata media

Change to not decode extra data when first loading a preload=metadata media. Once the user plays, we will preroll as usual. If you seek without playing first, we also not buffer unnecessary samples, though we will still decode to the seek target and discard samples before the seek target.

With this patch, we'll not preroll by default on all platforms, not only on mobile.

Green: https://tbpl.mozilla.org/?tree=Try&rev=b028258565e2
Attachment #512296 - Attachment is obsolete: true
Attachment #512347 - Attachment is obsolete: true
Attachment #520298 - Attachment is obsolete: true
Attachment #8397428 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 71

4 years ago
Comment on attachment 8397428 [details] [diff] [review]
Patch2 v1: Reduce preroll for preload=metadata media

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

Actually, I think we can solve this in a simpler way.
Attachment #8397428 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 72

4 years ago
Created attachment 8398158 [details] [diff] [review]
Patch 3v1: Don't preroll when media isn't playing

We can solve this problem with a simpler approach.

With my previous approach we didn't decode and buffer more than the first sample of all active streams if we were loading preload=metadata media element, unless the user played.

With this patch, we won't decode and buffer more than the first sample of all active streams unless we're playing. This means many play() operations will only have one audio/video samples prebuffered before we start playing. I tested on Tarako and an ASUS T100 (ATOM Z2760 CPU) and despite buffering only one sample before playback starts, I didn't see glitches when playback started. So I think this will probably be OK.

With this patch, we'll reduce our memory footprint, and speed up seeking when the media is paused (note: our videocontrols pause() before seek, and play() after seek) as we'll reduce the amount of frames we decode after seeking.
Attachment #8397428 - Attachment is obsolete: true
Attachment #8398158 - Flags: review?(cajbir.bugzilla)

Updated

4 years ago
Attachment #8398158 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 73

4 years ago
Landed for Gecko 31:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda301682977
(Assignee)

Updated

4 years ago
Whiteboard: [has-patch]
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9b4d9313e348 since I didn't know whether it was this, bug 778077, or both, causing Mac debug mochitest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36849618&tree=Mozilla-Inbound, Windows mochitest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850811&tree=Mozilla-Inbound, Android like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850825&tree=Mozilla-Inbound, and Windows reftest failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=36850065&tree=Mozilla-Inbound. So far.
(Assignee)

Comment 75

4 years ago
Comment on attachment 8398158 [details] [diff] [review]
Patch 3v1: Don't preroll when media isn't playing

Unfortunately, we can't use this simpler solution, as it affects our readyState logic, and the only way to make it work would be to fake our readyState logic. I think we'll have to go back to my previous solution.
Attachment #8398158 - Attachment is obsolete: true
(Assignee)

Comment 76

4 years ago
Comment on attachment 8397428 [details] [diff] [review]
Patch2 v1: Reduce preroll for preload=metadata media

With this patch, we'll only reduce our preroll when we're in preload=metadata, not always, as in my other patch.
Attachment #8397428 - Attachment is obsolete: false
Attachment #8397428 - Flags: review?(cajbir.bugzilla)

Comment 77

4 years ago
Comment on attachment 8397428 [details] [diff] [review]
Patch2 v1: Reduce preroll for preload=metadata media

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

::: content/media/MediaDecoderStateMachine.h
@@ +362,5 @@
>    // appropriate. The decoder monitor must be held while calling this.
>    void NotifyWaitingForResourcesStatusChanged();
>  
> +  // Notifies the state machine that should not decode samples that don't
> +  // need to be decoded.

This comment is hard to parse. It also seems to be missing a word unless I'm reading it wrong.

@@ +924,5 @@
>    // True if we should run the state machine again once the current
>    // state machine run has finished.
>    bool mRunAgain;
>  
> +  // True if we should not decode/preroll unnecessary samples, unless we're

Might want to define what 'preroll' means somewhere.
Attachment #8397428 - Flags: review?(cajbir.bugzilla) → review+
(Assignee)

Comment 78

4 years ago
With this patch I also need to change some webaudio tests to set prefs media.preload.default=2 and media.preload.auto=3. 

We can't rely on the usual way in manifest.js as that doesn't use pushPrefEnv, so the test can run before the pref changes have been communicated across processes in b2g and fennec builds.

I was seeing content/media/webaudio/test/test_mediaElementAudioSourceNode.html and content/media/webaudio/test/test_mediaStreamAudioSourceNodeResampling.html time out in b2g and android mtest-4, as in push: https://tbpl.mozilla.org/?tree=Try&rev=027ead2ee723

Fixed by setting the prefs, as in this push:
https://tbpl.mozilla.org/?tree=Try&rev=2c2e3c3746f0
(Assignee)

Comment 79

4 years ago
Relanded with fixes for Gecko 31:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35180f110e44
https://hg.mozilla.org/mozilla-central/rev/35180f110e44
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Updated

4 years ago
Depends on: 990908

Updated

4 years ago
Depends on: 991066
(Assignee)

Updated

4 years ago
Depends on: 993753
Depends on: 990841
You need to log in before you can comment on or make changes to this bug.