cajbir (:cajbir) Assignee
	Description • 16 years ago

The Ogg backend doesn't implement seeking so writing to currentTime does not work. This also affects any media behaviour that requires adjusting the seek point of the file (looping, etc).

	cajbir (:cajbir) Assignee
	Comment 1 • 16 years ago

This patch adds seeking support for local files URL's. Setting currentTime on a file://... resource should seek fine. Support for HTTP coming.

	cajbir (:cajbir) Assignee
	Comment 2 • 16 years ago

Adds support for seeking http resources using byte range requests. Fixes various issues related to seeking that came up. Adding test in a separate patch - they require httpd.js to support byte range requests which I need to look into.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 16 years ago

Argh, I wrote a long comment and then lost it :-(. Here we go again:

+  *aSeeking = mDecoder ? mDecoder->IsSeeking() : PR_FALSE;

mDecoder && mDecoder->IsSeeking()

Why aren't we firing "waiting" and "seeking" events? That should be easy enough.

+class nsStreamStrategy 

Need a comment explaining what this is for.

I think nsStreamStrategy and nsMediaStream and related code (including channel/pipe stuff) should be made Ogg-independent and reused by pretty much all of our backends. It seems to me that all of our backends will need this.

+  virtual nsresult Tell(PRInt64* aOffset) = 0;
+  virtual nsresult Available(PRUint32* aCount) = 0;
+  virtual nsresult DownloadRate(float* aRate) = 0;

Can these actually fail? If they can't, then we should make them return results directly.

What are the rules for thread-safe use of Close() in nsMediaStream and the strategy classes? It's not clear how these classes are thread-safe in general. You say Open and Close are called on the main thread and the other methods are called on the decoder thread, and presumably the decoder thread doesn't run until after Open has finished, but how do we know that Close() doesn't race with decoder-thread methods?

+nsresult nsFileStreamStrategy::DownloadRate(float* aRate)
+{
+  *aRate = -1.0;
+  return NS_OK;

Use a named constant instead of a magic number.

+  PRPackedBool mStopDecoder;

Need to document how this is synchronized, or rather, why it's safe to not synchronize access to it.

+  // PR_TRUE if we are currently seeking to a position in the media
+  // resource.
+  PRPackedBool mSeeking;

Need to document how access to this is synchronized. Also for the other variables here; it's not enough to just say who writes and who reads.

+    char* data = new char[bytesAhead];

Use nsAutoArrayPtr<char>.

+    MOZ_COUNT_CTOR(nsDecoderEvent);
   }
-
+  

Don't add trailing whitespace.

+      mReader->Available() < mReader->PlaybackRate()) {    

So the idea is to buffer when we have less than one second's data available?

+      if (event) 
+        // The pause event must be processed before the
+        // buffering loop occurs. Otherwise the loop exits
+        // immediately as it checks to see if the user chose
+        // to unpause. 
+        NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);

This doesn't need to be conditional on (event). Dispatch will just fail if event is null.

+      NS_GetCurrentThread()->Dispatch(event, NS_DISPATCH_NORMAL);

Use NS_DispatchToCurrentThread

+    if (event) 
+      NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

Don't need to test event. (Lots of other occurrences)

Do we have documentation for how A/V sync works? If we did, it seems like it would need to be updated here.

The following issues don't need to be addressed in this patch, but we should deal with them:

When a server doesn't support byte-range requests, we should read the stream from the beginning to the desired point, instead of just leaving it at 0.

But we should check whether the server supports byte-range requests by making the initial load a byte-range request. Then we can set the "seekable" attribute based on whether the server supports byte-ranges and whether we have an estimate for the duration. Then we can use step 6 of the seek algorithm spec to reject seek attempts for servers without byte-ranges, which would mean the fallback in the previous paragraph would hardly ever be needed.

For servers that can't seek (or even servers that can), we should look into saving more buffered data and supporting seeking within that data. This extension would be further out that then other items I'm mentioning.

The Ogg code should really move to its own directory. I propose reorganising the directories as follows:
content/media/video/public/* -> content/media
content/media/video/src/* -> content/media
content/media/video/test/* -> content/media/tests
except for Ogg-only stuff which would go to content/media/ogg.
What do you think?

	cajbir (:cajbir) Assignee
	Comment 4 • 16 years ago

> +  virtual nsresult Tell(PRInt64* aOffset) = 0;
> +  virtual nsresult Available(PRUint32* aCount) = 0;
> +  virtual nsresult DownloadRate(float* aRate) = 0;
> 
> Can these actually fail? If they can't, then we should make them return results
> directly.

Tell and Available fall back to xpcom io methods so they apparently can fail. For example, Tell fails if the stream is not seekable and Available fails if the stream is closed with a NS_BASE_STREAM_CLOSED. DownloadRate can't fail so I changed this. I also changed PlaybackRate.

> What are the rules for thread-safe use of Close() in nsMediaStream and the
> strategy classes? It's not clear how these classes are thread-safe in general.
> You say Open and Close are called on the main thread and the other methods are
> called on the decoder thread, and presumably the decoder thread doesn't run
> until after Open has finished, but how do we know that Close() doesn't race
> with decoder-thread methods?

The media stream is only read/seeked from by the decoder thread. The decoder thread is informed before closing that the stream is about to close via calls to the media stream Cancel() method and the oggplay_prepare_for_close (oggplay actually closes the reader which closes the stream) function. The latter results in oggplay internally not calling any read/write/seek/tell methods, and returns a value that results in stopping the decoder thread. oggplay_close is then called which results in the the media stream being closed.

I've now added comments about this.


> +      mReader->Available() < mReader->PlaybackRate()) {    
>
> So the idea is to buffer when we have less than one second's data available?

Yes, once less than one second of data is available, start buffering
until approximately BUFFERING_WAIT seconds are available.

> Do we have documentation for how A/V sync works? If we did, it seems like it
> would need to be updated here.

I've updated nsOggDecoder.h with notes on how a/v sync works.

I agree with all your remaining points about handling byte range requests and reorganising the directories. I'll add them to the list of things to be done.

Other changes since the review include:

- address shutdown issues with the seek event as discussed over whiteboard
- Fixed a bug in liboggz to do with seeking and handling errors. I mentioned the issue on the liboggz irc channel and will upstream it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 5 • 16 years ago

(In reply to comment #4)
> Tell and Available fall back to xpcom io methods so they apparently can fail.
> For example, Tell fails if the stream is not seekable

Can that actually happen? We created the stream, so I imagine that if the stream's not seekable we shouldn't even be calling Tell on it.

> and Available fails if
> the stream is closed with a NS_BASE_STREAM_CLOSED.

Can that happen? Why not just return 0 in that case?

> > What are the rules for thread-safe use of Close() in nsMediaStream and the
> > strategy classes? It's not clear how these classes are thread-safe in general.
> > You say Open and Close are called on the main thread and the other methods are
> > called on the decoder thread, and presumably the decoder thread doesn't run
> > until after Open has finished, but how do we know that Close() doesn't race
> > with decoder-thread methods?
> 
> The media stream is only read/seeked from by the decoder thread. The decoder
> thread is informed before closing that the stream is about to close via calls
> to the media stream Cancel() method and the oggplay_prepare_for_close (oggplay
> actually closes the reader which closes the stream) function. The latter
> results in oggplay internally not calling any read/write/seek/tell methods, and
> returns a value that results in stopping the decoder thread. oggplay_close is
> then called which results in the the media stream being closed.
> 
> I've now added comments about this.

Great thanks.

> > +      mReader->Available() < mReader->PlaybackRate()) {    
> >
> > So the idea is to buffer when we have less than one second's data available?
> 
> Yes, once less than one second of data is available, start buffering
> until approximately BUFFERING_WAIT seconds are available.

Please add a constant for that "one second" and multiply by it, so it's clear what's happening here.

> - address shutdown issues with the seek event as discussed over whiteboard
> - Fixed a bug in liboggz to do with seeking and handling errors. I mentioned
> the issue on the liboggz irc channel and will upstream it.

Thanks!

	cajbir (:cajbir) Assignee
	Comment 6 • 16 years ago

I've made the additional changes as suggested.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 16 years ago

+computes how long it has been since it was last called and sleeps for

since the last *call*

+#define MEDIA_UNKNOWN_RATE -1.0

NS_MEDIA_UNKNOWN_RATE

Need to document which fields of nsMediaStream are accessed on which threads.

+    nsAutoArrayPtr<char> data(new char[bytesAhead]);

We should check for OOM here.

+  if (NS_SUCCEEDED(rv) && bytesAhead > 0 && available >= static_cast<PRUint32>(bytesAhead)) {

Use constructor-style cast

We should add your liboggz change as a stand-alone patch file as well so it's clear how our copy of liboggz is generated from upstream.

	cajbir (:cajbir) Assignee
	Comment 8 • 16 years ago

The seek patch to liboggz will be in a separate patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 16 years ago

Comment on attachment 336773 [details] [diff] [review]
Patch for liboggz

Looks good, but I would also attach the change to oggz_seek.c as part of this patch, not the other patch, to avoid confusion.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 16 years ago

+  // Bytes downloaded for average playback rate computation. Initialized
+  // on the main thread during Open(). Written during Read() calls on any
+  // thread. Read from PlaybackRate() onthe same thread that it was written
+  // to during Read().
+  PRUint32 mPlaybackRateCount;

Needs to be documented that after Open(), this is read and written exclusively by *some* thread. Might as well mention that for Ogg, it's the decoder thread.

+  // Flag to indicate if the stream has been cancelled, and all i/o
+  // should fail. See comments on nsMediaStream::Cancel implementation
+  // for details. Written on the main thread during the Cancel() call. 
+  // Reading on the thread doing reading/seeking to cause an error when
+  // the stream is cancelled.
+  PRPackedBool mCancelled;

We discussed this. We should get rid of this flag or otherwise sort out its synchronization behaviour.

	cajbir (:cajbir) Assignee
	Comment 13 • 16 years ago

Addresses review comments. Also addresses issues discussed in person to do with synchronisation and deadlocking.

- Removed usage of Revoke() and locking in events.
- Fixed shutdown issues to do with shutdown of threads.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 16 years ago

So this is ready to land. However, we also need tests. I know that depends on getting httpd.js to do byte-range requests and stuff, so it will take some work, but I think that should be high priority.

	cajbir (:cajbir) Assignee
	Comment 15 • 16 years ago

Agreed, I'm on it. The shutdown issue was actually found while writing the tests :-)

	cajbir (:cajbir) Assignee
	Comment 16 • 16 years ago

Current trunk has changes from bug 451938 adding ability to the the current principal. This patch merges this in.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 17 • 16 years ago

Comment on attachment 337259 [details] [diff] [review]
Merge to latest trunk, picking up GetCurrentPrincipal addition

principals bit looks fine, thanks!

	cajbir (:cajbir) Assignee
	Comment 18 • 16 years ago

Tests seeking. Requires binary attachment and httpd byte range patch from bug 398185.

	cajbir (:cajbir) Assignee
	Comment 19 • 16 years ago

This patch contains a binary file. 'patch' won't handle it. So it needs to be applied with hg import --no-commit.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 16 years ago

What video is that? Can we just have an encoding of black/silence, that should compress to something small, I hope!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 16 years ago

Can we test currentTime on seekEnded to make sure it's right or at least sane?

Should we test for "waiting"?

I think we should use a long timeout --- test machines get slow and we really really need to avoid spurious orange. But we don't need to wait for the timeout under normal circumstances; we should be able to finish the test as soon as playbackEnded has fired. Or even seekEnded; do we really need to measure the remaining playback time? Is that to avoid bugs where currentTime reports what we've seeked to without actually playing from there?

	cajbir (:cajbir) Assignee
	Comment 22 • 16 years ago

Smaller binary file. 4 seconds of black with 4 seconds of blank audio.

	cajbir (:cajbir) Assignee
	Comment 23 • 16 years ago

Fixes a bug where currentTime and other metadata wasn't set by the time the seeked event was raised.

	cajbir (:cajbir) Assignee
	Comment 24 • 16 years ago

Adds test for currentTime in seeked event. I'm not currently testing for "waiting" since it only occurs if buffering will start during a seek which doesn't happen with the blazingly fast httpd.js. I'll need to think about how to do this. Possibly add the test when I do the remaining seek related tasks?

	cajbir (:cajbir) Assignee
	Comment 25 • 16 years ago

Comment on attachment 337269 [details] [diff] [review]
Binary file for tests

Revert back to using this ogg file. The manually generated one from png's turns out not to work correctly with some players, including ours.

	cajbir (:cajbir) Assignee
	Comment 26 • 16 years ago

Comment on attachment 337358 [details] [diff] [review]
Smaller binary file

Possibly broken file...looking into it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 16 years ago

So you've added this:

+    mPresentationThread->Dispatch(event, NS_DISPATCH_SYNC);

This happens on the main thread. It seems rather dodgy since, while we're waiting for the presentation thread to do its thing, we'll be running the main thread event loop and doing arbitrary things, such as tearing down the current document. In particular I suspect we can crash in the call to StartupDecoder if we've shut down and mDecodeThread is null.

Tomorrow let's talk about how to avoid this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 29 • 16 years ago

+#if !defined(nsMediaDecoder_h___)
+#define nsMediaDecoder_h___

Apparently identifiers with at least two trailing underscores are "reserved" for the compiler/platform. Just use 1. Ditto for nsMediaStream.h

It looks to me like nsHTMLVideoElement::GetVideoWidth/Height end up reading mRGBWidth/Height while not holding a lock. That's bad, but taking the lock isn't enough, because really we shouldn't be allowing the exposed video size to change except during a main-thread event. I thought we used to handle this correctly. But anyway, it seems to me that the element should have its own copy of the video size, which we should update from an event dispatched to the main thread.

+  nsAutoPtr<nsMediaStream> mStream;

Can mStream just be a directly embedded object?

+    This thread goes through the data decoded by the decode thread,
+    generates the RGB buffer.

"and generates"?

+playback of the resource. The event contains an nsMonitor which is
+  // internally managed via an nsMonitor.

PRMonitor or maybe say mMonitor.

+The following events result in state transitions.

These aren't XPCOM events, so just say "functions"?

+  | Shutdown()        v Seek(t) ^     v Buffer    ^    v Complete

There should be an arrow here for Shutdown

+The shutdown event can occur during any of the states to start an
+orderly shutdown of the thread.

According to your state diagram, we can only enter Shutdown from the DECODING state. So either we need to add more transitions to the diagram, or adjust this comment to explain what happens if we need to shutdown before we reach the DECODING state.

The nsOggDisplayStateMachine diagram needs edges to show the transitions to SHUTDOWN.

+This extra state was maintained, rather than reading the state from the Decode
+and Display state machines, as they can be different. For example, the high level
+state could be SEEKING but the Display state machine is PAUSED while seeking
+is happening. That PAUSED state should not be evident from users of the
+nsHTMLMediaElement object.

It would be good to say, as you explained to me, that the element state reflects what the client has requested through the API, whereas the state machine states represent what is actually happening.

-
+  

Don't add trailing whitespace.

+  // Provide access to the ogg metadata. Synchronised via the monitor.

Fix the comment to make it clear that this data will only be accessed after we've left the DECODING_METADATA phase, at which point it's read-only. Maybe add an assertion to that effect to each of the methods. Also, the getter methods should just be inline in the class.

IsCompleted and Load could also just be inline.

+  oggplay_prepare_for_close(mPlayer);
+  ChangeState(DECODER_STATE_SHUTDOWN);

Document that prepare_for_close is irreversible, so there's no way for oggplay to get out of the prepared-for-close state before we go into SHUTDOWN state.

+  mSeekTime = aTime;
+  ChangeState(DECODER_STATE_SEEKING);

How about here? I guess this is only called with the monitor already held. But that needs to be documented, and ideally, asserted.

Seems like we grab the decoder lock and then the state machine locks. That order really needs to be enforced to guarantee deadlock-freedom ... unless we find a way to make do with only one lock.

+  // Return PR_TRUE if we have completed decoding or are shutdown.
+  PRBool IsCompleted();

What is the thread-safety of this method?

> static_cast<ogg_int64_t>(mSeekTime * 1000)

Constructor-style cast.

+  while(num_tracks-- > 0)

Space "while ("

Think about whether it makes sense to have only one lock so we can be sure that transitions on multiple states can happen atomically and so that we're sure to be deadlock-free and things should be simpler. I think we can do it, we just need to make sure that the state machines drop the lock before they wait for anything other than a state change.

+        mon.Exit();
+        mDecodeStateMachine->Wait(PR_MillisecondsToInterval(10));
+        mon.Enter();

Seems like if we have a single lock we can easily just wait indefinitely on it here. (It seems like here you really want to wait on the OR of both monitors...)

+  // deleted). I don't obtain the monitor as two monitors cannot be
+  // nested without a deadlock assertion occuring.

This would be safer too.

It would be really nice if we could assert that a monitor is held by the current thread.

Seems to me that it would be slightly simpler to have nsOggDisplayStateMachine and nsOggDecodeStateMachine subobjects of nsOggDecoder. Just do an nsRunnableMethod dispatch to a forwarding method on nsOggDecoder to bootstrap the threads.

If we do get down to one lock, then I think it's worth exploring whether we can simplify the states. Right now it's not clear what the relationships are between the three states. I wonder if we could get down to one main state plus some extra state information for decode and for display. Probably, whatever we do, we should at least work out and document what all the possible state combinations are and what transitions are possible between them; then we can figure out the best way to represent them.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 16 years ago

We talked about the states with a whiteboard.

The decoder-thread states are
DECODING_METADATA
DECODING_FIRSTFRAME
DECODING
SEEKING
BUFFERING
COMPLETED
SHUTDOWN

The high-level player states are
START
LOADING
PLAYING
PAUSED
SEEKING
SHUTDOWN

The decoder-thread state and the player state are mostly orthogonal, for example because while the player is SEEKING the decoder may need to be decoding the first frame at the destination of the seek. Similarly the player may be PAUSED but the decoder is still BUFFERING or DECODING. I think the possible combinations are:
player LOADING: decoder DECODING_METADATA
player PLAYING: decoder DECODING_FIRSTFRAME, DECODING, BUFFERING, SEEKING, COMPLETED
player PAUSED: decoder DECODING_FIRSTFRAME, DECODING, BUFFERING, SEEKING, COMPLETED
player SEEKING: decoder DECODING_FIRSTFRAME, SEEKING, DECODING
player SHUTDOWN: decoder SHUTDOWN
This information should be corrected and then documented somewhere.

It seems the display thread doesn't really need states. It can play a frame whenever the oggplay queue has a frame and the player state is PLAYING. So it just needs to keep waiting on the monitor and checking the player state and the oggplay queue. The decoder will need to notify on the monitor when the oggplay queue might have changed from empty to non-empty.

	cajbir (:cajbir) Assignee
	Comment 31 • 16 years ago

This addresses the review comments and implements changes what we discussed over the whiteboard. I've added an extra player state 'COMPLETED' to represent when playback is completed. 'SHUTDOWN' remains for when the decoder is being destroyed. Adding COMPLETED allowed restarting completed playing. I think we discussed this state was needed at one point.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 16 years ago

 nsIntSize nsHTMLVideoElement::GetVideoSize(nsIntSize defaultSize)

aDefaultSize

Might want to add comments to nsOggDecoder.h explaining why two extra threads is the minimum number of threads we need.

+Note that these play states are the states requested by the client
+through the api. They are the state that will go into affect by the
+other threads when they are able to. This is different from the decode
+state machine where those states are the actual currently occurring
+state.

The wording is a bit clumsy. how about

"The player states are the states requested by the client through the DOM API. They represent the desired state of the player, while the decoder's state represents the actual state of the decoder."

+player SEEKING   decoder SEEKING

Is this correct? I thought the decoder could be DECODING the first frame after the seek while the player is still SEEKING.

+   step is done, so it can do the required things by the video
    step has been done, once again so it can do the required things by

"things required"

+   That transition gets the Display state machine to start display
+   data if the PLAYING state is in effect and the Decode machine into
+   DECODING.

"That transition causes the Display thread to start displaying data if the PLAYING state is in effect and the decoder is DECODING."

   /****** 
    * The following methods must only be called on the main
    * thread.
    ******/
-
+  

Don't add trailing whitespace.

+  // Monitor for detecting when the video play state changes. A call
+  // to Wait on this monitor will block the thread until the video 
+  // pauses, unpauses or shutsdown.
+  PRMonitor* mMonitor;

Actually the thread would block just until the next state change in the decoder, play state, or oggplay queue, right?

+  This object keeps pointers the the nsOggDecoder and nsChannelReader

"keeps pointers to the"

+  // Cause state transitions. Can be called from any
+  // thread. Synchronisation is internally managed via obtaining the
+  // decoder's monitor.
+  void Shutdown();
+  void Decode();
+  void Seek(float aTime);

Seems like Decode and Seek are always called with the monitor held. Probably simpler just to say that the monitor must be held and ensure that that's true (I think you'd just need to add one monitor grab around the call to Shutdown).

+      if (mState == DECODER_STATE_DECODING_METADATA)
+        mState = DECODER_STATE_DECODING_FIRSTFRAME;

{} around these single statemennts, please

+          nsCOMPtr<nsIRunnable> event = 
+            NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, BufferingStarted);
+          NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

You should fire this event after we've actually gone into the BUFFERING state.

+          if (r == E_OGGPLAY_USER_INTERRUPT) {
+            // Buffer will block on the next call if the display thread
+            // doesn't display a frame beforehand.
+            mon.Wait();

We want to avoid blocking at all costs, right? So this doesn't seem like enough. Someone could notify on our monitor for a reason other than "display thread displayed a frame", then we'd exit this Wait, go around the loop and go back to call DecodeFrame while the buffer was full. We may need a boolean flag, protected by the state monitor, which indicates whether the Ogg queue is full, which we always check before calling DecodeFrame and call Wait instead if it's set.

+        nsCOMPtr<nsIRunnable> startEvent = 
+          NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, SeekingStarted);
+        NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);

This is weird. We do a sync dispatch to the main thread, which just does an async dispatch also to the main thread, which then does *another* async dispatch of the DOM event! I guess we need to be on the main thread in order to use mElement, but why can't SeekingStarted() just call nsHTMLMediaElement::SeekStarted as a direct call? Same goes for SeekingStopped.

I like that we don't send more than one event to the decoder thread, so we don't have to worry about what happens on the decoder thread while we're doing the sync dispatch to the main thread. You might want to explain that nothing can happen during the sync calls.

I think SeekingStarted and SeekingStopped both need to grab the decoder lock and check that we're not in SHUTDOWN state before doing any work. Right now we could be be firing seeking-related DOM events during Shutdown, which seems bad.

+        oggplay_seek(mPlayer, ogg_int64_t(mSeekTime * 1000));

Here you access mSeekTime without holding the lock, which is bad. I think you should copy its value to a local variable before you exit the monitor.

+        if (mState == DECODER_STATE_SEEKING)
+          mState = DECODER_STATE_DECODING;

So what if Seek() was called while this seek was already in progress? It seems that here we're just dropping that seek on the floor. How about checking mState == DECODER_STATE_SEEKING *and* that mSeekTime still has the value we started out with? Then, if another seek is requested during this seek and the destination is different, we'll correctly seek again. Probably should write a test for that.

+  // A pointer to the decode state machine. This is used to access
+  // resource metadata like framerate, audio channels, etc. It is also
+  // used when there is no decoded data available to block on the
+  // decode state machines state change so we know decode data is
+  // possibly available. Synchronisation is handled by the decoder
+  // monitor. This object is destroyed by mDecoder during Shutdown,
+  // but after we receive the SHUTDOWN state transition so it is safe
+  // to use anytime before we receive SHUTDOWN.
+  nsOggDecodeStateMachine* mDecodeStateMachine;

Can't you just get this through mDecoder?

+  // Initializes and opens the audio stream. Called from the display
+  // thread only.

Also document that this must be called with the monitor held so we don't race with SetVolume.

+      if (state != prevState) {
+        // Just changed to play state, resume audio.

I think it would be more obvious to keep a local variable "audioPlaying" and start and stop the audio by checking that variable.

Shutdown remains a problem, although we shouldn't fix it in this bug. Basically when the element is destroyed we have to change the decoder's state to SHUTDOWN, remove the element reference from the decoder and post an XPCOM event to shutdown the decoder later.

+  if (mPlayState == PLAY_STATE_SEEKING && aTime == mSeekTime)
+    return NS_OK;

We shouldn't really need to check this, so don't.

-   if (mAudioTrack != -1 && num_tracks > mAudioTrack) {
-      ProcessTrack(mAudioTrack, track_info[mAudioTrack]);
-   }
+  if (mPlayState == PLAY_STATE_LOADING && aTime == 0.0)
+    return NS_OK;
+
+  mSeekTime = aTime;
+  mNextState = mPlayState;
+  ChangeState(PLAY_STATE_SEEKING);

Seems to me we should only set mNextState if mPlayState is PLAYING or PAUSED (e.g. not SEEKING).

Should PLAY_STATE_COMPLETED be PLAY_STATE_STOPPED?

-  if (mElement) {
+  if (mElement)
     mElement->FirstFrameLoaded();
-  }

Don't remove {}

-  if (mElement) {
+  if (mElement)
     mElement->ResourceLoaded();
-  }

Ditto

-  if (mElement) {
+  Stop();
+  if (mElement) 
     mElement->PlaybackCompleted();
-  }

Ditto

-  if (mElement) {
+  if (mElement)
     mElement->ChangeReadyState(nsIDOMHTMLMediaElement::CAN_SHOW_CURRENT_FRAME);
-    mElement->Play();
-  }

Ditto

-  if (mElement) {
-    mElement->Pause();

Ditto

	cajbir (:cajbir) Assignee
	Comment 33 • 16 years ago

> Is this correct? I thought the decoder could be DECODING the first frame after
> the seek while the player is still SEEKING.

It is correct. The decoding of the first frame, and the presentation of that frame is handled while still the state is SEEKING as part of the seek operation. This ensures that when the seeked event is raised that it has the correct time, etc. 

> Seems to me we should only set mNextState if mPlayState is PLAYING or PAUSED
> (e.g. not SEEKING).

Need to handle the case of the user seeks, the seek is in progress, then they seek again. That's why mNextState is set.

This doesn't handle the issue of if they do more than once operation during a seek (play, then seek, then pause). I'll get clarified what should happen here spec-wise.

> Should PLAY_STATE_COMPLETED be PLAY_STATE_STOPPED?

Playback can't be stopped, only paused. COMPLETED is for when the video has completed playing. ENDED is probably the better term to match the corresponding event in HTML5. If you have no objection I'll change to that.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 34 • 16 years ago

> > Seems to me we should only set mNextState if mPlayState is PLAYING or PAUSED
> > (e.g. not SEEKING).
> 
> Need to handle the case of the user seeks, the seek is in progress, then they
> seek again. That's why mNextState is set.
> 
> This doesn't handle the issue of if they do more than once operation during a
> seek (play, then seek, then pause). I'll get clarified what should happen here
> spec-wise.

I think we should remember the last seek request, if any, and the last play/pause request. When we finish the current seek we apply the next seek, if any, and then the play/pause request.

	cajbir (:cajbir) Assignee
	Comment 35 • 16 years ago

Ok, I'll do that and adjust if needed when the spec is clarified.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 36 • 16 years ago

The Run() methods of nsOggDecodeStateMachine and nsOggDisplayStateMachine must be declared as "NS_IMETHOD Run();" not "nsresult Run();" else you get a calling convention mismatch error when compiling on Windows.

+// All methods of nsVideoDecoder must be called from the main thread only

I think you meant nsMediaDecoder.

	cajbir (:cajbir) Assignee
	Comment 38 • 16 years ago

Adds suggested test as per review

	cajbir (:cajbir) Assignee
	Comment 39 • 16 years ago

Adds changes suggested by Chris

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 40 • 16 years ago

I can cause a deadlock with attachment 339929 [details] [diff] [review] (and 336974) applied.

STR:
1. Open http://pearce.org.nz/player/
2. Click "BillysBrowser480.ogg" from available media.
3. Press "Toggle" button once, so that "Feedback: no" is shown.
4. Play the video, seek to 10 seconds into video by entering 10 into "seek" field, and pressing the lightning-icon button. It should work.
5. Press "Toggle" so that "Feeback: yes" is shown. This starts a self-resetting timeout which updates playback info about the video.
6. Try seeking again, browser will deadlock.

When I look at a stack trace in a debugger, the main thread is in nsHTMLVideoElement::GetCurrentTime() - my javascript timeout is calling this, which causes the deadlock somehow.

Once these patches land, I'll file this as a new bug, just thought you'd appreciate knowing about it sooner rather than later...

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 41 • 16 years ago

Also, if controls are on, once a video has played through, and it automatically seeks to the beginning, if you play the video again using the inbuilt controls, the play button still appears drawn over the video frame throughout the entire playback. This is a regression from this patch - the directshow backend could already seek, and the play button used to be hidden correctly after replaying the video a second time using the controls.

	cajbir (:cajbir) Assignee
	Comment 42 • 16 years ago

Chris, see bug 449154 for issues related to re-playing after playback is completed.

	cajbir (:cajbir) Assignee
	Comment 43 • 16 years ago

Small patch to fix issues raised by Chris Pearce in comments 40 and 41.

	cajbir (:cajbir) Assignee
	Comment 45 • 16 years ago

This fixes various deadlock issues and moves to using only one thread by removing the display thread, and folding it into the decode thread. Fix shutdown issues where the event loop was spun at bad times by doing the actual shutdown in an event queued to the main thread.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 47 • 16 years ago

One question is what should happen when someone tries to seek to +Inf. I guess,
per spec, we should only be allowing seeking within the known duration limit,
so currentTime = +Inf would be synchronously rejected as not within the limit.
So I suppose in followup work we'll have to compute the duration
asynchronously, with seeking not permitted at all until we know the duration,
and once we know the duration +Inf will be rejected as greater than the
duration. OK.

Either add a test for media errors, or move the media error stuff to a separate
patch.

+void nsHTMLMediaElement::MediaErrorDecode()
+{
+  mError = new nsHTMLMediaError(nsHTMLMediaError::MEDIA_ERR_DECODE);
+  mBegun = PR_FALSE;
+  DispatchProgressEvent(NS_LITERAL_STRING("error"));
+  mNetworkState = nsIDOMHTMLMediaElement::EMPTY;
+  DispatchSimpleEvent(NS_LITERAL_STRING("emptied"));
+  if (mDecoder) {
+    mDecoder->Shutdown();
+    mDecoder = nsnull;
+  }

OK, so what if the JS spawned by the "error" event whacks the state of this
element and calls load() before returning, or some similar state-whacking? You
probably need some check here that before we proceed to "emptied" and
Shutdown(), we should keep doing those things. Whatever you do, please write a
test for that.

+FrameData object. This is stored in queue of available decoded frames.

/in queue/in a queue

+move to synchronising off the audio hardware.

/off/using

+To prevent audio skipping and framerate dropping it is very important to
+make sure no blocking occurs during the decoding process and minimising

/minimising/minimise

 ifdef MOZ_OGG
 ifeq ($(OS_ARCH),Linux)
-DEFINES += -DSYDNEY_AUDIO_NO_POSITION
+# DEFINES += -DSYDNEY_AUDIO_NO_POSITION
 endif

Why are we commenting this out? Should we just delete it?

I think we should remove nsMediaDecoder::StartInvalidating/StopInvalidating
since you're not actually using them.

Document that nsMediaDecoder::SetRGBData must be called with mVideoUpdateLock
held.

Why can't SetRGBData just take ownership of the data buffer that's passed in?
That would avoid data copying and probably simplify your code a tiny bit since
you'd no longer have to allocate in SetRGBData. But give it malloc/free
allocation so that we don't get mismatched allocators.

+    // is cancelled. Check for to see if we are cancelled

Check for what?

+    // Handle cases of aWhence not being NS_SEEK_SET but converting to

/but/by

+    if (NS_SUCCEEDED(rv) && bytesAhead > 0 && diff > -SEEK_VS_READ_THRESHOLD)
{

Add a comment explaining your "read instead of seek" strategy here.

+      nsresult rv = mPipeInput->Read(data.get(), bytesAhead, &bytes);
+      mPosition += bytesAhead;

Only increment mPosition if the Read succeeded.

I assume that if there's a Cancel() during the synchronous read, we get an
error rv here and bail out. Please add a comment if that's the case.

+  nsCOMPtr<nsByteRangeEvent> event = new nsByteRangeEvent(this, mDecoder,
mURI, aOffset);
+  NS_DispatchToMainThread(event, NS_DISPATCH_SYNC);
+
+  mPosition = aOffset;
+  return event->GetResult();

What happens if someone Cancels() us during the sync dispatch? What if there's
some kind of error, we wouldn't want to set mPosition would we? Please add
comments.

+  // Handle the case of a seek to EOF by liboggz
+  // (See Seek for details)
+  if (mEOFPosition != -1) {
+    PRInt64 result = mEOFPosition;
+    mEOFPosition = -1;

Resetting mEOFPosition here seems wrong. If you call Tell twice, you should get
the same position, right?

+// Download rate can take some time to stabalise. For the buffering

/stabalise/stabilise

+// Time in seconds to pause while buffering video data
+// on a slow connection.
+#define BUFFERING_WAIT 15

So, this means that we wait 15 seconds and then leave buffering and play as
best we can? Make the comment say that.

+// Download rate can take some time to stabalise. For the buffering
+// computation there is a minimum rate to ensure a reasonable amount
+// gets buffered.
+#define BUFFERING_MIN_RATE 50000
+#define BUFFERING_RATE(x) ((x)< BUFFERING_MIN_RATE ? BUFFERING_MIN_RATE : (x))

I'm not really sure what that comment means either

+// The number of seconds of buffer data before buffering happens
+// based on current playback rate.
+#define BUFFERING_SECONDS_WATERMARK 1

So this means if we have less then 1 second of data to play, start buffering?
In that case, call it BUFFERING_SECONDS_LOW_WATER_MARK. Wouldn't want anyone to
think we're detecting watermarks or something :-).

More coming...

	cajbir (:cajbir) Assignee
	Comment 48 • 16 years ago

A couple of lines were indented incorrectly.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 16 years ago

In FrameQueue, instead of storing mFull, just have IsFull check !mEmpty &&
mHead == mTail.

FrameData should have MOZ_COUNT_CTOR/MOZ_COUNT_DTOR so we can detect any leaks
of frames.

+  // Decode one frame of data, returning the OggPlay error code. Must
+  // be called only when the current state > DECODING_METADATA. The decode 
+  // monitor does not need to be locked during this call since liboggplay
+  // internally handles locking.

Shouldn't this say that the decode monitor MUST NOT be locked since this call
can take a while?

+  // DECODING_METADATA. DisplayTrack returns the timestamp, in
+  // seconds, of the track that was processed.
+  float HandleTrack(FrameData* aFrame, int aTrackNumber, OggPlayCallbackInfo*
aTrack);

You mean HandleTrack not DisplayTrack in the comment.

It would be good to reorganize the fields of nsOggDecodeStateMachine so that
they're grouped by access rules --- i.e., fields accessed under the lock,
fields accessed only by main thread, fields accessed only by decoder thread.

+  PRIntervalTime mSyncTime;
+  PRPackedBool mPlaying;
+  float mCallbackPeriod;

Comment what these fields mean.

+  mDecodedFrames(OGGPLAY_BUFFER_SIZE),

Since this is a constant, why not have the FrameQueue contain a fixed-size
array directly instead of heap-allocating it?

+  FrameData* frame = new FrameData();
+  frame->mTime = mLastFrameTime;

OOM check

Why does HandleTrack need to switch on the track type? We should already know
whether we expect video or audio data in the track.

+  aFrame->mVideoData = new unsigned char[y_width * y_height * 4];

OOM check

+    float* newData = new float[newSize];

OOM check. But actually, it might make more sense for FrameData::mAudioData to
be an nsTArray<float> --- easier to grow.

+      mSyncTime += PR_IntervalNow();

This is confusing, the units must be wrong. Are you trying to treat mSyncTime
as both a timestamp and an interval between timestamps? Would probably be less
confusing using two variables.

+        mDecodedFrames.Pop();
+        PlayVideo(mDecodedFrames.IsEmpty() ? frame : mDecodedFrames.Peek());

Why would it be correct to display the next frame? Shouldn't we look through
the queue to find the last frame whose time is < the current time, and display
that one?

+        while (r == E_OGGPLAY_TIMEOUT) {
+          r = DecodeFrame();
+        }

Make it do ... while to avoid the initial known-true check.

+          if (mDecoder->GetState() != nsOggDecoder::PLAY_STATE_PLAYING)
+            mon.Wait();

You need {} around that statement. You don't need {} around your 'continue's.

+        nsCOMPtr<nsIRunnable> event =
+          NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, PlaybackEnded);
+        NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+        mon.Wait();
+        if (mState == DECODER_STATE_SHUTDOWN) {
+          continue;
+        }

Shouldn't this be do { mon.Wait(); } while (mState != DECODER_STATE_SHUTDOWN)?
We don't want to be woken up spuriously and then do "case
DECODER_STATE_COMPLETED" again.

+        while (mState != DECODER_STATE_SHUTDOWN) {
+          mon.Wait();
+        }

do ... while.

I think LoadFirstFrame should be inlined into the decoder main loop. I think we
should check the state for SHUTDOWN after each DecodeFrame --- it would be
clearer if that was in the main loop. You also wouldn't need another 'mon' ---
basically, LoadFirstFrame is a lot like the DECODE or first-frame-after-seek
cases and it should be alongside that code.

+  rv = mDecodeThread->Dispatch(mDecodeStateMachine, NS_DISPATCH_NORMAL);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  ChangeState(PLAY_STATE_LOADING);

I think you should put the ChangeState before the Dispatch. The reason is that,
as written, the ChangeState could occur either before or after
mDecodeStateMachine has started. It would be more predictable to ensure that
the ChangeState always runs before mDecodeStateMachine has started (even if it
shouldn't matter). (You can also then just tail-call Dispatch for slightly
simpler code.)

+    if (mSeekTime >= 0.0)
+      ChangeState(PLAY_STATE_SEEKING);
+    else
+      ChangeState(mNextState);

{} around these ChangeStates.

Two classes have an 'mSeekTime'. To reduce confusion, it would be good to give
them different names.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 16 years ago

The main thread doesn't do any monitor waits, does it? Blocking the main thread like that would be bad if we did it, so I'm glad we don't. But in that case, we don't need to do any monitor notifies in the decoder thread, so take those out.

	cajbir (:cajbir) Assignee
	Comment 51 • 16 years ago

I removed the media error stuff and will put in a separate patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 52 • 16 years ago

> +      nsresult rv = mPipeInput->Read(data.get(), bytesAhead, &bytes);
> +      mPosition += bytesAhead;
> 
> Only increment mPosition if the Read succeeded.
>
> I assume that if there's a Cancel() during the synchronous read, we get an
> error rv here and bail out. Please add a comment if that's the case.

These still needs to be fixed.

+  aFrame->mAudioData.AppendElements((float*)aAudioData, size);

reinterpret_cast

+  // Time interval used for synchronising frames. It represents the
+  // interval on the system clock from when playback started.
+  // Accessed only via the decoder thread.
+  PRIntervalTime mSyncTime;
+
+  // Time interval to adjust mSyncTime by to account for when the
+  // playback was paused. Accessed only via the decoder thread.
+  PRIntervalTime mPauseTime;

mSyncTime and mPauseTime are still confusing, and the comments aren't very helpful. Now, mPauseTime is sometimes a (negative) moment in time and sometimes a difference between times. Those are really different types. It would be clearer if you separated it into mCurrentPauseStartTime and mPauseDuration. Likewise mSyncTime is sometimes zero and sometimes the time of the first frame after a seek, but we always use it so I don't know how that's supposed to work.

+        OggPlayErrorCode r = E_OGGPLAY_TIMEOUT;
+        do {
+          r = DecodeFrame();

No need to initialize r to E_OGGPLAY_TIMEOUT (occurs twice).

+          mon.Enter();
+          if (mState == DECODER_STATE_SHUTDOWN)
+            break;
+          mon.Exit();
+        } while (r == E_OGGPLAY_TIMEOUT);
+
+        mon.Enter();

Looks like if mState is DECODER_STATE_SHUTDOWN we Enter mon twice. What I think we need to do here is Exit just before calling DecodeFrame and Enter just after (also occurs twice).

+        } while(mState != DECODER_STATE_SHUTDOWN);

"while ("

+          if (mState != DECODER_STATE_SHUTDOWN && mDecoder->GetState() != nsOggDecoder::PLAY_STATE_PLAYING) {

80 column warning

+  // The decoder monitor must be obtained before modifying this state.
+  // NotifyAll on the monitor must be called when state is changed so
+  // other threads can react to changes.
+  State mState;

Fix comment to say that NotifyAll must be called when the state is changed *by the main thread* so that the decoder thread can wake up and react to changes.

+  All internal state is synchronised via the decoder
+  monitor. NotifyAll on the monitor is called when the state of the
+  state machine is changed. The following changes to state cause a

Similar here.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 54 • 16 years ago

Comment on attachment 343539 [details] [diff] [review]
Address review comments

+  // The time that playback started from the system clock. This is used
+  // for synchronising frames. It includes any time taken for
+  // pausing and buffering.

I would just remove the sentence "It includes" ... it's confusing.

+  // The total time that has been spent paused (via pause or buffering)
+  // since playback or a seek was started.

I would say "the total time that has been spent in completed pauses (via 'pause' or buffering)." to make it clear time in a "current" pause is not included here.

Overall that's much better though.

+        mon.Exit();
+
+        nsCOMPtr<nsIRunnable> event =
+          NS_NEW_RUNNABLE_METHOD(nsOggDecoder, mDecoder, FirstFrameLoaded);
+        NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
+
+        mon.Enter();

We don't need to exit the monitor here, because it's not a sync dispatch.

+  // Set to one of the valid play states. It is protected by the
+  // monitor mMonitor. This monitor must be acquired when reading or
+  // writing the state. Any change to the state must call NotifyAll on
+  // the monitor.
+  PlayState mPlayState;

This comment about NotifyAll still needs to be fixed.

Make those changes and check it in!

	cajbir (:cajbir) Assignee
	Comment 56 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/0051e26b159e
http://hg.mozilla.org/mozilla-central/rev/f941129cae01

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 57 • 16 years ago

Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20081212 Shiretoko/3.1b3pre ID:20081212035014 and Beta 2 on OS X.

Tests are available here:
http://mxr.mozilla.org/mozilla-central/source/content/media/video/test/

As Clint said on IRC some more will probably added.