For the implementation of a camera input stream (bug 507749), encoding to ogg+theora only to decode is wasting a lot of cpu. The solution I see is to ship raw rgb internally and to support this I have implemented a raw "decoder." This is minimal implementation and there are several functions that are no-ops. I am not sure if any of these are required. Another problem is there is no way that I know of to get the height, width, aspect ratio and frame rate from the raw stream, so I added that functionality to the camera input stream and use that in the raw decoder.

What happens if a web page sends data with this mime type? What does the decoder do given that there is no frame rate, aspect ratio, width, height or means to identify frames in the file?

I suspect that with the current patch we'd fail miserably. I'd be looking to you for a suggestion on how to allow web content to utilize this mimetype. Perhaps allowing the width, height, framerate and aspect ratio to be set as attributes of the video tag would work.

Comment on attachment 392304 [details] [diff] [review] patch v.1 I don't have any suggestions in the area of providing this data from web content. I don't think we should add custom attributes to the video element. This could cause problems when it clashes with data provided by other codecs. People might just copy/paste a raw example and expect the declared frame rate, etc to work. Is there some other, already defined, 'raw' format that can be used for this purpose, which has this data as part of the stream. Perhaps something like: http://en.wikipedia.org/wiki/OggUVS Do you need to deal with Audio from the camera? If so, how does this fit into the stream? If using a container like Ogg (in OggUVS or similar) at least audio and other data could be transmitted.

If you aren't interested in audio, YUV4MPEG could be an option: http://wiki.multimedia.cx/index.php?title=YUV4MPEG2

updated to work with current layers implementation. Doesn't address review comments.

this implements reading in an oggyuv stream (as defined here: http://wiki.xiph.org/OggYUV)

Comment on attachment 445471 [details] [diff] [review] patch +#ifdef MOZ_RAW +static const char gRawTypes[][16] = { + "video/x-raw", + "video/x-raw-rgb" +}; This (and other canPlayType handling in the patch) will need minor updates for the changes made to how canPlayType works in bug 566245 attachment 448322 [details] [diff] [review]. --- a/content/media/Makefile.in Fri May 07 16:48:33 2010 -0400 +ifdef MOZ_RAW +DIRS += raw +endif The other backends use PARALLEL_DIRS, why does raw use DIRS? +++ b/content/media/raw/nsRawDecoder.h Mon Aug 03 14:59:42 2009 -0400 +class nsRawStreamListener : public nsIStreamListener { ... +class nsRawDecoder : public nsMediaDecoder Instead of handling reading from the stream using a listener I suggest using nsMediaStream. This gives a file like abstraction to the stream and allows seeking in parts of the stream that have been cached amongst other benefits. Deriving from nsMediaDecoder requires implementing a whole pile of stuff to get the correct HTML5 spect events right. I suggest deriving from nsBuiltinDecoder and friends. For a smallish example see bug 566245 attachment 448326 [details] [diff] [review]. This implements the WebM decoder and uses much of the shared functionality in the nsBuiltin base classes. This will give you the correct events, buffering, and other features for free. + PRBool mPositionChangeQueued; Write a comment on what this is for. + PRBool mPositionChangeQueued; + PRInt32 mWidth; + PRInt32 mHeight; + double mFramerate; + double mAspectRatio; Reorder these so the PRBool is the last one declared and make it a PRPackedBool. + PRUint32 mPlaybackPosition; + PRUint32 mDownloadPosition; + clock_t mInitialTime; + nsRefPtr<nsIStreamListener> mListener; + nsMediaStream* mStream; Document what the threading requirements are for these (main thread only?). Instead of using clock_t use mozilla::TimeStamp or mozilla::TimeDuration. +++ b/content/media/raw/nsRawDecoder.cpp Mon Aug 03 14:59:42 2009 -0400 +nsRawStreamListener::nsRawStreamListener(nsRawDecoder* aDecoder) +{ + mDecoder = aDecoder; + mPositionChangeQueued = PR_FALSE; + mFramerate = mAspectRatio = 0.0; + mWidth = mHeight = 0; +} Move these outside of the constructor body. eg: nsRawStreamListener::nsRawStreamListener(nsRawDecoder* aDecoder) : mDecoder(aDecoder), mPositionChangeQueued(PR_FALSE), ... { } + aStream->ReadSegments(&NS_CopySegmentToBuffer, buf, count, &read); handle return value of ReadSegments. + if (buf[0] == 0) { // that's our header packet Need to check that at least 1 byte was read before indexing into buf. + memcpy(&mWidth, buf + 12, 3); + memcpy(&mHeight, buf + 15, 3); Use of 'magic numbers' should be moved into constants or #define's at the top of the file. Same with the rest of the file. Also document where the numbers come. This copies only 3 bytes into a a PRInt32? What is the 4th byte set to? How do you deal with endian issues (PPC). Same goes with the other memcpy code later. + mAspectRatio = aspect_n / aspect_d; + mFramerate = framerate_n / framerate_d; Check for aspect_d and framerate_d being zero to avoid divide by zero issues. + int offset = mWidth * mHeight / 4; Check for overflow during mWidth * mHeight. Offset should be NSPR type, PRUint32. + data.mCbChannel = buf + offset * 4 + data_header_size; + data.mCrChannel = buf + offset * 5 + data_header_size; offset is calculated by multiplying two values (width and height) that are read from the bitstream. These values are not sanitized and are added to a memory pointer. Need to check the values for 'valid' ranges and check the arithmetic for overflows. + data.mCbCrSize = data.mYSize / 2; + data.mCbCrStride = data.mYStride / 2; Is the YCbCr format from the file always 4:2:0? Do you need to deal with 4:2:2 and 4:4:4? + clock_t ct = clock(); UpdatePlaybackPosition((ct - mDecoder->mInitialTime) / 1000); TimeStamp::Now() and other Mozilla time clode instead of clock(). +void nsRawDecoder::GetCurrentURI(nsIURI**) +{ +} + +already_AddRefed<nsIPrincipal> nsRawDecoder::GetCurrentPrincipal() +{ + return nsnull; +} This and other unimplemented functions will need to be implemented. If you base code on the nsBuiltin* classes you get much of it for free. +float nsRawDecoder::GetCurrentTime() +{ + clock_t ct = clock(); + float t = ((float)(ct - mInitialTime)) / (float)CLOCKS_PER_SEC; + return t; + +} Use mozilla time functions (and in other places as well).

I forgot to mention that it needs tests. At least a simple playback test. bug 566245 attachment 448321 [details] [diff] [review] show's how a simple playback test can be added.

I'm going to take it from here and address Chris's review comments.

Rebuilt on nsBuiltinDecoder* as suggested, with tests.

Comment on attachment 450734 [details] [diff] [review] Patch +++ b/content/html/content/src/nsHTMLMediaElement.cpp +static PRBool IsRawEnabled() +{ + return PR_TRUE; +} This should be based on a pref like the webm and ogg backends. The following files need the MPL license header: content/media/raw/Makefile.in content/media/raw/nsRawDecoder.h content/media/raw/nsRawDecoder.cpp +++ b/content/media/raw/nsRawReader.h + PRBool ReadFromStream(nsMediaStream *stream, PRUint8 *buf, PRUint32 l); Make argument names match standard (aStream, aBuf, etc). diff --git a/content/media/raw/nsRawReader.cpp b/content/media/raw/nsRawReader.cpp + +static PRBool MulOverflow32(PRUint32 a, PRUint32 b, PRUint32 &aResult) +{ ... +} This appears to be used by the Ogg and Raw backends. Can it be moved into VideoUtils.h and shared? +nsresult nsRawReader::ReadMetadata() + [...] + if (!ReadFromStream(stream, (PRUint8*)&mMetadata, sizeof(mMetadata))) Use C++ style casts (static_cast, reinterpret_cast, etc). + mInfo.mPixelAspectRatio = mMetadata.aspectNumerator / mMetadata.aspectDenominator; Need a cast to float at least on of aspectNumerator or aspectDenominator here otherwise the result of the division is an integer division. + mFramerate = mMetadata.framerateNumerator / mMetadata.framerateDenominator; Need a cast to float at least on of framerateNumerator or framerateDenominator here otherwise the result of the division is an integer division. In general we'll need to validate all the metadata values read from the stream to be within sane ranges so calculations used by them don't cause problems. + mDecoder->GetStateMachine()->SetDuration(1000 * + (length - sizeof(nsRawVideoHeader)) / + (mMetadata.frameWidth * mMetadata.frameHeight * + (mMetadata.lumaChannelBpp / 8.0 + mMetadata.chromaChannelBpp / 8.0) + + sizeof(nsRawPacketHeader)) / + mFramerate); Some lines longer than 80 characters. The arithmetic can overflow (frameWidth * frameHeight). Division by zero is possible if mFramerate is 0 or if the 2nd division return is 0. +// Helper method that either reads until it gets l bytes or returns PR_FALSE +PRBool nsRawReader::ReadFromStream(nsMediaStream *stream, PRUint8* buf, PRUint32 l) Should be aStream, aBuf, etc. +{ + nsresult rv = NS_OK; 'rv' isn't used outside of the loop. What it entered to return 'rv'? If not, put it in the loop as part of the 'Read' call. + + while (NS_SUCCEEDED(rv) && l > 0) { + PRUint32 bytesRead = 0; + rv = stream->Read((char*)buf, l, &bytesRead); Use C++ style cast. Need to check 'rv' return value. +PRBool nsRawReader::DecodeVideoFrame(PRBool &aKeyframeSkip, + PRInt64 aTimeThreshold) +{ + [...] + PRInt64 currentFrameTime = 1000 * mCurrentFrame / mFramerate; Division by zero if mFramerate is zero. This goes back to the need to check the validity of the metadata that was read previously. + PRUint32 length = mMetadata.frameWidth * mMetadata.frameHeight * + (mMetadata.alphaChannelBpp + mMetadata.lumaChannelBpp + + mMetadata.chromaChannelBpp) / 8; Overflow possibilities here. + nsAutoPtr<PRUint8> buffer(new PRUint8[length]); 'length' is calculated using values read from the stream that haven't been checked for valid values. The values should perhaps be constrained to be within sane ranges. + // We're always decoding one frame when called + while(true) { + // printf("Reading packet!\n"); + nsRawPacketHeader header; + + // Read in a packet header and validate + if (!(ReadFromStream(stream, (PRUint8*)&header, sizeof(header))) || C++ style casts please. + !(header.packetID == 0xFF && header.codecID == 0x595556 /* "YUV" */)) { + return PR_FALSE; + } The 0x595556 is used in a couple of places. Move it out into a #define at the toplevel of the .cpp file. + b.mPlanes[1].mHeight = mMetadata.frameHeight / 2; + b.mPlanes[1].mWidth = mMetadata.frameWidth / 2; The code assumes 4:2:0? There should probably be a check/assertion somewhere about this. +nsresult nsRawReader::Seek(PRInt64 aTime, PRInt64 aStartTime, PRInt64 aEndTime) + [...] + PRUint32 frame = mCurrentFrame; + mCurrentFrame = aTime * mFramerate / 1000; mCurrentFrame is a PRUint32 while aTime is a PRInt64. Overflow possibility here in the aTime*mFramerate calculation. + + PRUint32 offset = sizeof(nsRawVideoHeader) + mCurrentFrame * + (sizeof(nsRawPacketHeader) + + (mMetadata.alphaChannelBpp + mMetadata.lumaChannelBpp + + mMetadata.chromaChannelBpp) / 8.0 * + mMetadata.frameWidth * mMetadata.frameHeight); Overflow checking needed. + rv = stream->Seek(nsISeekableStream::NS_SEEK_SET, offset); Check return value of seek. + { + mozilla::MonitorAutoExit autoMonitorExit(mMonitor); + mozilla::MonitorAutoEnter autoMonitor(mDecoder->GetMonitor()); + if (mDecoder->GetDecodeState() == nsBuiltinDecoderStateMachine::DECODER_STATE_SHUTDOWN) { + mCurrentFrame = frame; + return NS_ERROR_FAILURE; + } + } Nested incorrectly.

Updated to comments. In particular, this clamps the frame size to reasonable values which ensures that most calculations cannot overflow.

Are you sure you uploaded the right patch? A cursory look shows review comments not addressed (for example, still no MPL header on nsRawReader.h, etc).

Comment on attachment 451710 [details] [diff] [review] Patch updated to review comments Yeah this isn't the right version.

Comment on attachment 451794 [details] [diff] [review] Right patch In nsRawDecoder.h and nsRawDecoder.cpp: + * Contributor(s): + Might want to add yourself as a contributor.

Backed out for Bug 573161 http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f3a6e20989fc

Comment on attachment 451794 [details] [diff] [review] Right patch We need this for the camera work.

Comment on attachment 451794 [details] [diff] [review] Right patch a=beltzner

http://hg.mozilla.org/mozilla-central/rev/070072f39303 Relanded. Will track orange if it reappears in Bug 573161.

This patch added this variable: 1214 static const char* gRawMaybeCodecs[] = { 1215 nsnull 1216 }; which isn't used anywhere, as shown by this MXR search: http://mxr.mozilla.org/mozilla-central/search?string=gRawMaybeCodecs That line should probably be removed, right? (or is it used in a not-yet-landed followup bug?)

(In reply to comment #22) > That line should probably be removed, right? er, s/that line/that variable/

It looks like the maybeCodecs stuff went away between when I wrote the patch and when I landed it.

Added OggYUV to the list of supported codecs here: https://developer.mozilla.org/En/Media_formats_supported_by_the_audio_and_video_elements I've seen notes that say this isn't used in an Ogg container, but the OggYUV page seems to say it is, so that's how I've documented it. If that's not correct, let me know.