Closed Bug 508082 Opened 10 years ago Closed 10 years ago

Implement raw decoder


(Core :: Audio/Video, defect)

Not set





(Reporter: blassey, Assigned: khuey)


(Blocks 1 open bug)


(Keywords: dev-doc-complete, mobile)


(1 file, 5 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
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.
Attachment #392304 - Flags: review?(chris.double)
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:

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.
Attachment #392304 - Flags: review?(chris.double) → review-
If you aren't interested in audio, YUV4MPEG could be an option:
Attached patch patch (obsolete) — Splinter Review
updated to work with current layers implementation.  Doesn't address review comments.
Attachment #392304 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
this implements reading in an oggyuv stream (as defined here:
Attachment #445416 - Attachment is obsolete: true
Attachment #445471 - Flags: review?(chris.double)
Attachment #445471 - Flags: review?(chris.double) → review-
Comment on attachment 445471 [details] [diff] [review]

+#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/	Fri May 07 16:48:33 2010 -0400
+ifdef MOZ_RAW
+DIRS += raw

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) :

+  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

+      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.
Assignee: nobody → me
Attached patch Patch (obsolete) — Splinter Review
Rebuilt on nsBuiltinDecoder* as suggested, with tests.
Attachment #445471 - Attachment is obsolete: true
Attachment #450734 - Flags: review?(chris.double)
Comment on attachment 450734 [details] [diff] [review]

+++ 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:


+++ 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.
Attachment #450734 - Flags: review?(chris.double) → review-
Attached patch Patch updated to review comments (obsolete) — Splinter Review
Updated to comments.  In particular, this clamps the frame size to reasonable values which ensures that most calculations cannot overflow.
Attachment #450734 - Attachment is obsolete: true
Attachment #451710 - Flags: review?(chris.double)
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.
Attachment #451710 - Attachment is obsolete: true
Attachment #451710 - Flags: review?(chris.double)
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.
Attachment #451794 - Flags: review?(chris.double) → review+
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Comment on attachment 451794 [details] [diff] [review]
Right patch

We need this for the camera work.
Attachment #451794 - Flags: approval2.0?
Comment on attachment 451794 [details] [diff] [review]
Right patch

Attachment #451794 - Flags: approval2.0? → approval2.0+

Relanded.  Will track orange if it reappears in Bug 573161.
Target Milestone: mozilla2.0b1 → mozilla2.0b3
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 582328
This patch added this variable:
1214 static const char* gRawMaybeCodecs[] = {
1215   nsnull
1216 };

which isn't used anywhere, as shown by this MXR search:

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.
Depends on: 587479
Added OggYUV to the list of supported codecs here:

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.
No longer blocks: 451674
Blocks: camera
You need to log in before you can comment on or make changes to this bug.