Closed Bug 560708 Opened 10 years ago Closed 10 years ago

Factor out non-Ogg specific parts of nsOggDecoder.cpp

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch Refactor nsOggDecoder (obsolete) — Splinter Review
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,
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attachment #440404 - Flags: superreview?(roc)
Attachment #440404 - Flags: review?(chris)
Depends on: 551378
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.
Attached patch Patch 1: Rename/Move files (obsolete) — Splinter Review
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.
Attachment #440404 - Attachment is obsolete: true
Attachment #440443 - Flags: superreview?(roc)
Attachment #440443 - Flags: review?(chris)
Attachment #440404 - Flags: superreview?(roc)
Attachment #440404 - Flags: review?(chris)
Attached patch Patch 2: Refactor (obsolete) — Splinter Review
Refactor's as per the original patch but incorporating review comments (renaming to nsBuiltinDecoder, etc).
Attachment #440444 - Flags: superreview?(roc)
Attachment #440444 - Flags: review?(chris)
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.
As requested, patch done as an hg mv.
Attachment #440443 - Attachment is obsolete: true
Attachment #440455 - Flags: superreview?(roc)
Attachment #440455 - Flags: review?(chris)
Attachment #440443 - Flags: superreview?(roc)
Attachment #440443 - Flags: review?(chris)
Attached patch Patch 2: Refactor (obsolete) — Splinter Review
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.
Attachment #440444 - Attachment is obsolete: true
Attachment #440456 - Flags: superreview?(roc)
Attachment #440456 - Flags: review?(chris)
Attachment #440444 - Flags: superreview?(roc)
Attachment #440444 - Flags: review?(chris)
Attachment #440455 - Flags: superreview?(roc) → superreview+
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 <chris.double@double.co.nz>
>- *  Chris Pearce <chris@pearce.org.nz>
>  *


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 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 <chris.double@double.co.nz>
- *  Chris Pearce <chris@pearce.org.nz>
  *

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?
Attachment #440456 - Flags: superreview?(roc) → superreview+
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.
Attachment #440456 - Attachment is obsolete: true
Attachment #440659 - Flags: superreview+
Attachment #440659 - Flags: review?(chris)
Attachment #440456 - Flags: review?(chris)
Attachment #440659 - Flags: review?(chris) → review+
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.
(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.
Attachment #440455 - Flags: review?(chris) → review+
Landed, unbitrotten:
http://hg.mozilla.org/mozilla-central/rev/5d89cbfa2053
http://hg.mozilla.org/mozilla-central/rev/2bd54675c370
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.