[Bug 412647]

[Chris Double]

Comment on attachment 462048
gstreamer changes alone in single patch

>+# The Initial Developer of the Original Code is the Mozilla Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2007

Initial developer should be Mozilla Foundation and update copyright to
2010. This change should be done to all the license headers included in
the patch.

>+// Define the max size of video elements in pixels
>+// Copied from the OGG decoder (Jan 29 - 2010)
>+#define MAX_VIDEO_WIDTH  4000
>+#define MAX_VIDEO_HEIGHT 3000

This should probably be declared somewhere in content/media so it can be
shared between the decoders.

>+// Define how much data we should try to read at a time,
>+// changing the value can potentially decrease or increase performance.
>+// Feel free to experiment
>+#define GST_BUFFER_READ_CHUNK_SIZE 1024*16

Should have brackets: (1024*16)

>+// Check that we are compatible with the GStreamer version (we wish to support
>+// the build system for the N800, but can't actually run on something older
>+// than 0.10.25 (as in the Nokia N900)
>+#if 10 > GST_VERSION_MINOR || (22 > GST_VERSION_MICRO && 10 == GST_VERSION_MINOR)
>+#error We do not support versions of the GStreamer library previous to 0.10.22 \
>+  while building, and previous to 0.10.25 when running.
>+#endif

Is this checked in 'configure' as well?

>+  mUnknownTypeSignal = g_signal_connect( mDecodeBin,
>+                                         "unknown-type",
>+                                         G_CALLBACK(DecodeBinUnknownType),
>+                                         this);

Remove spacing after the bracket and first function parameter. This
happens in another place too.

>+void nsGStreamerDecodeBin::DecodeBinNewPadAdded(void *,
>+                                                GstPad * aPad,
>+                                                gboolean aIsLast,
>+                                                nsGStreamerDecodeBin * aMe)

Align the '*' immediately after the type name. ie. no spacing:

GstPad* aPad,
nsGStreamerDecodeBin* aMe)

Make this change everywhere this appears.

>+{
>+  // Check pre-conditions
>+  PR_ASSERT(aMe);
>+  PR_ASSERT(aMe->mPadDest);

Why use PR_ASSERT instead of NS_ equivalents?

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

Align arguments.

>+  // Forwarder function to enable access to a protected member of the base class
>+  void SetVideoData(PRInt32 aWidth,
>+                    PRInt32 aHeight,
>+                    float aFramerate,
>+                    float aAspectRatio,
>+                    Image *aImage)
>+                    {
>+                      gfxIntSize aSize;
>+                      aSize.width=aWidth;
>+                      aSize.height=aHeight;
>+
>+                      nsMediaDecoder::SetVideoData(aSize,
>+                                                   aAspectRatio,
>+                                                   aImage);
>+                    };

Drop { .. } to align with 'void'.

>+  // Define cleanup macro
>+#define NSGSTREAMER_INPUT_BIN_INIT_RETURN_FAILURE(arg) \
>+  NS_GSTREAMER_LOG(("Failure due to : " arg )); \
>+  Shutdown(); \
>+  return PR_FALSE;

I'm not a fan of declaring macros inside functions since their scope is
global. Put this at the top of the file in global scope. It can have a
shorted name too.

>+  // Retrieve the src pad of the app source element
>+  GstPad *pad = gst_element_get_static_pad(mAppSrc, "src");
>+
>+  if(!pad) {
>+    NSGSTREAMER_INPUT_BIN_INIT_RETURN_FAILURE("Couldn't get static pad from "
>+                                              "mAppSrc element\n");
>+  }
>+
>+  // Create a ghost pad
>+  GstPad *ghostPad = gst_ghost_pad_new("src", pad);
>+
>+  if(!ghostPad) {
>+    NSGSTREAMER_INPUT_BIN_INIT_RETURN_FAILURE("Couldn't create ghost pad\n");
>+  }

The 'pad' needs to be unrefed here since the macro results in a return.

>+  // Add the ghost pad to the input bin
>+  if(!gst_element_add_pad(GST_ELEMENT_CAST(mInputBin), ghostPad)) {
>+    NSGSTREAMER_INPUT_BIN_INIT_RETURN_FAILURE("Couldn't attach pad to bin");
>+  }

Does the ghostPad need to be unrefed here?

>+  // Obtain lock
>+  PR_Lock(mElementWrapperLock);
>+  // Store the element wrapper
>+  mElementWrapper = aElementWrapper;
>+  // Release lock
>+  PR_Unlock(mElementWrapperLock);

For this and all other uses of PR_Lock/PR_Unlock, use nsAutoLock.

>+    // The data source data can be read from
>+    nsMediaStream *mNsMediaStream;

No need for the 'Ns' in the name. Just call it mMediaStream or something
like that.

>+    // TODO: Add some thread protection to the below two memebers

Spelling of 'members'

>+    // See if we can initialize the lib
>+    if (gst_init_check(0, 0, &gstreamerErrorText)) {

gst_init_check has this requirement:

 "This function should be called before calling any other GLib
functions"

How are you managing this?

>+      if (0 == major && 10 == minor && 25 <= micro) {

Throughout the GStreamer code checks are done this way, with the value
on the left hand side of the boolean operator. None of the other media
code does it this way and it's jarring to move between the two different
styles. I'd prefer it if you changed all the checks to match existing
usage.

>+  if (!mPipeline) {
>+    NS_WARNING("Function called without a sucessfull init first");
>+    return NS_ERROR_UNEXPECTED;
>+  }

Can this (and everywhere else it is repeated) just be an NS_WARN_IF or
similar macro?

>+// Dump pipeline to .dot file
>+//  GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(static_cast<GstElement*>(*mPipeline)), GST_DEBUG_GRAPH_SHOW_STATES, 

"PipelineOutput");
>+// Dump pipeline to stdout
>+  NS_GSTREAMER_LOG(("The pipeline:\n"));
>+  if (getenv("GST_PIPE"))
>+    DumpGStreamerElement(static_cast<GstElement*>(*mPipeline));

Make this included in a debug only build

>+// TODO:  Remove the below global variables, they are for debug only
>+int sSetRGBCalled = 0;
>+int sDraws = 0;

Define for debug mode only?

>+class nsGStreamerPipeline :
>+  private nsGStreamerDecodeBinPadInterface,
>+  public nsISupports
>+{
> ...
>+    // Get the lenght of the stream (if available)
>+    float GetDuration();

length in comment spelt wrong.

>+
>+  // Pass the data on
>+  aMe->mVideoSinkInterface->SetVideoData(
>+      GST_VIDEO_SINK_WIDTH(aMe),
>+      GST_VIDEO_SINK_HEIGHT(aMe),
>+      aMe->mFpsN / aMe->mFpsD,
>+      1.0,
>+      GST_BUFFER_DATA(aBuffer));

Can aMe->mFpsD be zero here? If so, need to guard against divide by
zero.

>+  if (!gst_structure_get_int(structure, "width", &width) ||
>+      !gst_structure_get_int(structure, "height", &height)) {
>+    GST_WARNING_OBJECT(fennecvideosink,
>+                       "invalid caps for buffer allocation %"
>+                       GST_PTR_FORMAT,
>+                       aCaps);
>+    return GST_FLOW_NOT_NEGOTIATED;
>+  }
>+
>+
>+  // Allocate a buffer with new
>+  PRUint8 *newBuffer = new PRUint8 [aSize];

Is there a need for a safety check here to ensure that very large videos don't cause OOM? Or is that already handled 
earlier by a caller?

Have you run the video mochitests? I get crashes when building with only
the gstreamer backend and running the tests.

Since this patch was originally written the media code was refactored to
make writing backends easier. In particular generic code exists to
handle raising the correct events at the right times. It would be a lot
of work but you might like to consider refactoring on top of that code.
See the Raw and WebM backends for examples. This doesn't need to be done
to get review or landed I just thought I'd mention it.

-- 
Firefox is not able to play mp4 <video> tags
https://bugs.launchpad.net/bugs/412647
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla.