Bug 560708 factored out the non-Ogg parts of nsOggDecoder into nsBuiltinDecoder. This bug is to factor out the non-Ogg parts of nsOggPlayStateMachine so that people writing other video decoders don't need to deal with handling audio/video queuing, decoder states, etc.

Renames nsOgg* files to nsBuiltin*

Factor's nsOggPlayStateMachine and nsOggReader into a codec independant part (nsBuiltinDecoderStateMachine and nsBuiltinDecoderReader)

Basically looks good to me. The comments below are mostly cosmetic with a few exceptions. + // Make the decoder state machine update the playback position. Called by + // the reader on the decoder thread (Assertions for this checked by + // mDecoderStateMachine. Missing closing ) Mention that it must be called with the decode monitor held. + void UpdatePlaybackPosition(PRInt64 aTime) { mDecoderStateMachine->UpdatePlaybackPosition(aTime); } wrap at 80 columns + // Return the current decode state + nsDecoderStateMachine::State GetDecodeState() { return mDecoderStateMachine->GetState(); } This must have some locking/threading requirements? +VideoData* nsBuiltinDecoderReader::FindStartTime(PRInt64 aOffset, PRInt64& aOutStartTime) +Data* nsBuiltinDecoderReader::DecodeToFirstData(DecodeFn aDecodeFn, + MediaQueue<Data>& aQueue) Fix indent + unsigned char* mData[3]; PRUint8* slightly better + struct YCbCrBuffer { + unsigned char* mData[3]; + PRUint32 mWidth[3]; + PRUint32 mHeight[3]; + PRUint32 mStride[3]; Wouldn't it make more sense to have some kind of Plane struct with mData/mWidth/mHeight/mStride and have an array of three Planes? That's the setup we used to have, but you've changed it for some reason? - MonitorAutoEnter mon(mMonitor); + mozilla::MonitorAutoEnter mon(mMonitor); add typedef mozilla::MonitorAutoEnter MonitorAutoEnter to the enclosing class to avoid this. + mozilla::Monitor mMonitor; And add a typedef to avoid this too. + // Decodes one page of audio data, enqueuing the audio data in mAudioQueue. + // Returns PR_TRUE when there's more audio to decode, PR_FALSE if the + // audio is finished, end of file has been reached, or an un-recoverable + // read error has occured. + virtual PRBool DecodeAudioPage() = 0; What exactly does "Page" mean in the world of generic containers? Does it just mean some arbitrary chunk of data now? Or will we define what it means for each container? + // Reads and decodes one video frame. Packets with a timestamp less than aTimeThreshold + // will be decoded (unless they're not keyframes and aKeyframeSkip is PR_TRUE), but will + // not be added to the queue. + virtual PRBool DecodeVideoPage(PRBool &aKeyframeSkip, Is it really one video frame, or is it one page? using mozilla::MonitorAutoExit; +using mozilla::MonitorAutoEnter; +using mozilla::TimeStamp; +using mozilla::TimeDuration; Should just have "using namespace mozilla;" here + mozilla::Monitor mMonitor; Use another typedef + LOG(PR_LOG_DEBUG, ("Loading Ogg Headers")); Fix this message since it might not be Ogg + // Get the duration from the Ogg file. Fix this comment + mozilla::Monitor mAudioMonitor; + mozilla::TimeStamp mBufferingStart; use typedefs

Look good. >diff --git a/content/media/ogg/nsOggReader.cpp b/content/media/ogg/nsOggReader.cpp [...] >+ >+ return res; >+} >+ >+ >+// Returns PR_TRUE when all bitstreams in aBitstreams array have finished >+// reading their headers. >+static PRBool DoneReadingHeaders(nsTArray<nsOggCodecState*>& aBitstreams) { >+ for (PRUint32 i = 0; i < aBitstreams .Length(); i++) { >+ if (!aBitstreams [i]->DoneReadingHeaders()) { >+ return PR_FALSE; >+ } >+ } >+ return PR_TRUE; >+} >+ >+ Cosmetic: two blank lines between functions here where the rest has one line. >+void nsBuiltinDecoderStateMachine::LoadMetadata() As dicussed, we may be able to push the "seek to end to get duration" approach down into the nsOggReader::ReadMetadata(). We sometimes mention packets in nsBuiltinDecoderReader. Maybe substitute with "frame/sample"? >+ // YCbCr data obtained from decoding the video. The index's are: >+ // 0 = Y >+ // 1 = Cb >+ // 2 = Cr >+ struct YCbCrBuffer { >+ unsigned char* mData[3]; >+ PRUint32 mWidth[3]; >+ PRUint32 mHeight[3]; >+ PRUint32 mStride[3]; >+ }; We never need to iterate over these, so why store them as arrays? My vote's in for a struct with three member Plane structs, mY, mCb, mCr. Then you also don't need to remember the indicies of each plane, it's defined in its name.

Address review comments. >What exactly does "Page" mean in the world of generic containers? Does it just >mean some arbitrary chunk of data now? Or will we define what it means for each >container? I changed this to mean 'unspecified amount of audio data' and 'a frame of video data' and changed the method names to match. >As dicussed, we may be able to push the "seek to end to get duration" approach >down into the nsOggReader::ReadMetadata(). I've left this for now and plan to address this when we add the first non-ogg backend. Currently it should actually work to get the end duration if the other backend implements the protocol. I suspect we may end up making it an Ogg only thing though. >We never need to iterate over these, so why store them as arrays? My vote's in >for a struct with three member Plane structs, mY, mCb, mCr. Then you also don't >need to remember the indicies of each plane, it's defined in its name. Unfortunately we do iterate over these in a number of places (to copy the data for example). I've done a combination of your approach and Robert's. I create a Plane structure and have an array of three of these.

mozilla-central: http://hg.mozilla.org/mozilla-central/rev/7519657a1586 http://hg.mozilla.org/mozilla-central/rev/25f7e6fc9008