560708 - Factor out non-Ogg specific parts of nsOggDecoder.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

The Ogg audio/video backend has a lot of non-Ogg specific parts related to decoding video. The intent of this bug is to move the parts of nsOggDecoder that can be shared across other backends into a common base class.

Comment 1

[cajbir (:cajbir)]

Attachment: Refactor nsOggDecoder

Moves the non-Ogg specific stuff from nsOggDecoder into an nsStandardDecoder (needs better name?). The code that runs in the state machine thread is based on an nsDecoderStateMachine base class which gets implemented by the backend. A future patch will factor out the non-ogg specific parts of that,

Comment 2

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

How about nsBuiltinDecoder? Meaning that it's the decoder we use for codecs and containers whose support is built into Gecko.

nsVideoHacks.h -> VideoUtils.h

Would it be possible to make this patch a rename from nsOggDecoder.h/.cpp to nsBuiltinDecoder.h/.cpp, followed by creation of nsOggDecoder.h/.cpp with the correct contents? I think that would produce a much smaller and easier to review patch.

Comment 3

[cajbir (:cajbir)]

Attachment: Patch 1: Rename/Move files

Renames nsOggDecoder.cpp/nsOggDecoder.h/nsOggHacks.h to nsBuiltinDecoder.h/nsBuiltinDecoder.cpp/VideoHacks.h as per preview comment. Patch 2 sits on top of this to make review easier.

Comment 4

[cajbir (:cajbir)]

Attachment: Patch 2: Refactor

Refactor's as per the original patch but incorporating review comments (renaming to nsBuiltinDecoder, etc).

Comment 5

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

you could actually prepare patch 1 as an hg rename. This would make the patch much smaller and preserve history. If you did that, combining the two patches would be fine.

Comment 6

[cajbir (:cajbir)]

Attachment: Patch 1: Rename/Move files

As requested, patch done as an hg mv.

Comment 7

[cajbir (:cajbir)]

Attachment: Patch 2: Refactor

Patch regenerated as requested. I couldn't combine both into one patch as mercurial wouldn't treat nsOggDecoder.cpp as a new file, instead it took it as modifing the existing version.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 440456 [details] [diff] [review]
Patch 2: Refactor


In general, looks good. Tests still pass on Windows. nsBuiltinDecoder decoder is a clunky name. Ideally we'd move everything from nsBuiltinDecoder up into nsMediaDecoder, and factoring common code from the wave backend into there too. Not in this patch though.


>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>index afd11c3..a6f74a2 100644
>--- a/content/media/nsBuiltinDecoder.cpp
>+++ b/content/media/nsBuiltinDecoder.cpp

>-nsresult nsOggDecoder::Load(nsMediaStream* aStream,
>+nsresult nsBuiltinDecoder::Load(nsMediaStream* aStream,
>                             nsIStreamListener** aStreamListener)

Indentation of arguments.


>-void nsOggDecoder::MetadataLoaded()
>+void nsBuiltinDecoder::MetadataLoaded()
>[...]
>   if (!mResourceLoaded) {
>     StartProgress();
>-  } else if (mElement) {
>+  }
>+  else if (mElement) {
>     // Resource was loaded during metadata loading, when progress
>     // events are being ignored. Fire the final progress event.
>     mElement->DispatchAsyncProgressEvent(NS_LITERAL_STRING("progress"));
>   }

Brace-before-"else" usage is inconsistent with usage elsewhere in file and tree. Change all of them, or none of them please.


> 
>-NS_IMETHODIMP nsOggDecoder::Observe(nsISupports *aSubjet,
>+NS_IMETHODIMP nsBuiltinDecoder::Observe(nsISupports *aSubjet,
>                                     const char *aTopic,
>                                     const PRUnichar *someData)

Indentation of arguments.

> 
>-void nsOggDecoder::NotifyBytesConsumed(PRInt64 aBytes)
>+void nsBuiltinDecoder::NotifyBytesConsumed(PRInt64 aBytes)
> {
>   MonitorAutoEnter mon(mMonitor);
>-  NS_ASSERTION(mDecodeStateMachine->OnStateMachineThread() || mDecodeStateMachine->OnDecodeThread(),
>+  NS_ASSERTION(OnStateMachineThread() || mDecoderStateMachine->OnDecodeThread(),
>                "Should be on play state machine or decode thread.");

Would it make sense to have an OnDecodeThread() method to match OnStateMachineThread()? Or will that not make sense after the next refactoring patch?


>diff --git a/content/media/nsBuiltinDecoder.h b/content/media/nsBuiltinDecoder.h
>index 4f5c74b..d4b31cf 100644
>--- a/content/media/nsBuiltinDecoder.h
>+++ b/content/media/nsBuiltinDecoder.h
>@@ -16,130 +16,53 @@
>  * The Original Code is Mozilla code.
>  *
>  * The Initial Developer of the Original Code is the Mozilla Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 2007
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *  Chris Double <[e-mail]>
>- *  Chris Pearce <[e-mail]>
>  *


I'd say it's polite to leave the names of people who make contributions to a file in the contributor list.

> #include "prlog.h"
>+#include "prmon.h"


We don't need to include prmon.h here, or in nsOggReader.h either incidently.

 
>diff --git a/content/media/ogg/nsOggPlayStateMachine.h b/content/media/ogg/nsOggPlayStateMachine.h
>index cff645e..b300dbc 100644
>--- a/content/media/ogg/nsOggPlayStateMachine.h
>+++ b/content/media/ogg/nsOggPlayStateMachine.h
>@@ -31,37 +31,169 @@
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
>+/*
>+Each video element for an Ogg file has two additional threads beyond
>+those needed by nsBuiltinDecoder.
>+
>+  1) The Audio thread writes the decoded audio data to the audio
>+     hardware.  This is done in a seperate thread to ensure that the
>+     audio hardware gets a constant stream of data without
>+     interruption due to decoding or diplay. At some point


diplay -> display

Comment 9

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

Comment on attachment 440456 [details] [diff] [review]
Patch 2: Refactor

+nsresult nsBuiltinDecoder::Load(nsMediaStream* aStream,
                             nsIStreamListener** aStreamListener)

Fix indent

-  } else if (mElement) {
+  }
+  else if (mElement) {

Don't do this!
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures

-    } else {
+    }
+    else {

Or this!

  *  Chris Double <[e-mail]>
- *  Chris Pearce <[e-mail]>
  *

Say what?

+static inline PRBool IsThread(nsIThread* aThread) {

Maybe IsCurrentThread?

+  virtual PRBool OnStateMachineThread() = 0;
+  virtual PRBool OnDecodeThread() = 0;

Can these be devirtualized and inlined by hoisting nsIThread pointers into nsDecoderStateMachine?

Comment 10

[cajbir (:cajbir)]

Attachment: Patch 2: Refactor, addressing review comments

Carried roc's r+ forward. Addresses review comments:

(In reply to comment #8)
> Ideally we'd move everything from nsBuiltinDecoder up into
> nsMediaDecoder, and factoring common code from the wave backend into there too.

Yes, it's not being done at this stage of the refactor so the GStreamer backend people don't need to rewrite their backend.

> I'd say it's polite to leave the names of people who make contributions to a
> file in the contributor list.

Apologies for this, I copy/pasted the header from another file at some point. It was not intentional.

(In reply to comment #9)
> Can these be devirtualized and inlined by hoisting nsIThread pointers into
> nsDecoderStateMachine?

I devirtualized OnStateMachineThread as it was never called from nsBuiltinDecoder anyway. I left OnDecodeThread for the refactoring of nsOggPlayStateMachine and nsDecoderStateMachine which is the next stage of the backend refactoring.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 440659 [details] [diff] [review]
Patch 2: Refactor, addressing review comments

(In reply to comment #10)
> Created an attachment (id=440659) [details]
> Patch 2: Refactor, addressing review comments
> > I'd say it's polite to leave the names of people who make contributions to a
> > file in the contributor list.
> 
> Apologies for this, I copy/pasted the header from another file at some point.
> It was not intentional.

No problem. I worded this comment poorly, it was unduly snarky, sorry for that.

Comment 12

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

(In reply to comment #8)
> Ideally we'd move everything from nsBuiltinDecoder up into
> nsMediaDecoder, and factoring common code from the wave backend into there
> too.

I'm actually not sure about that. Yes, it would be nice to share more code between nsWaveDecoder and nsBuiltinDecoder. But to integrate a completely different playback framework like GStreamer etc, I think we'd want to subclass nsMediaDecoder.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Landed, unbitrotten:
http://hg.mozilla.org/mozilla-central/rev/5d89cbfa2053
http://hg.mozilla.org/mozilla-central/rev/2bd54675c370