[Bug 412647]

[Chris Double]

Comment on attachment 605705
nsBuiltinDecoder* based implementation

Review of attachment 605705:
-----------------------------------------------------------------

To build on linux the patch needs some gstreamer header files added to
config/system-headers:

diff --git a/config/system-headers b/config/system-headers
index c0f0221..1486194 100644
--- a/config/system-headers
+++ b/config/system-headers
@@ -1055,3 +1055,7 @@ ogg/os_types.h
 nestegg/nestegg.h
 cubeb/cubeb.h
 #endif
+gst/gst.h
+gst/app/gstappsink.h
+gst/app/gstappsrc.h
+gst/video/video.h

The patch looks great. I've made some comments. The 'r-' is mostly minor
stuff, and because you've got more to do to complete it. I'm definitely
happy with the direction and approach of the patch though. You'll want
to add some .mp4 specific files in content/media/tests btw, and make
sure tests pass. This might be challenging if gstreamer is handling the
webm/ogg/wave, so I'm wondering if it's better than these are handled by
the internal code even if gstreamer is enabled.

::: content/media/gstreamer/Makefile.in
@@ +31,5 @@
> +# 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 *****

MPL boilerplate for new files should use the new MPL 2 text. This should
be done for all new files that you've added. Don't change the
boilerplate of existing files you're just modifying.
http://www.mozilla.org/MPL/headers/

::: content/media/gstreamer/nsGStreamerReader.cpp
@@ +116,5 @@
> +}
> +
> +nsresult nsGStreamerReader::Init(nsBuiltinDecoderReader* aCloneDonor)
> +{
> +  gst_init_check(0, 0, NULL);

Check return value and maybe disable gstreamer support if it fails.

@@ +124,5 @@
> +        "stream-type", GST_APP_STREAM_TYPE_SEEKABLE,
> +        "max-bytes", 51200,
> +        NULL));
> +  gst_app_src_set_callbacks(mSource, &mSrcCallbacks, (gpointer) this, NULL);
> +  mDecodebin = gst_element_factory_make("decodebin2", NULL);

Is it needed to handle the failure case of gst_element_factory_make (and
similar functions) when they return NULL, or does GStreamer gracefully
handle the later use of NULL elements?

@@ +224,5 @@
> +      GST_FORMAT_TIME, timestamp);
> +  timestamp = GST_TIME_AS_USECONDS(timestamp);
> +  PRInt64 duration;
> +  if (GST_CLOCK_TIME_IS_VALID(GST_BUFFER_DURATION(buffer)))
> +    duration = GST_TIME_AS_USECONDS(GST_BUFFER_DURATION(buffer));

If this fails, duration will be uninitialized?

@@ +226,5 @@
> +  PRInt64 duration;
> +  if (GST_CLOCK_TIME_IS_VALID(GST_BUFFER_DURATION(buffer)))
> +    duration = GST_TIME_AS_USECONDS(GST_BUFFER_DURATION(buffer));
> +  PRInt32 frames = (GST_BUFFER_SIZE(buffer) / 4) / mInfo.mAudioChannels;
> +  PRInt64 offset = 0;

Might need to check for mInfo.mAudioChannels being zero here, or when it
is initially set in the preroll callback later.

@@ +231,5 @@
> +  
> +  LOG(PR_LOG_ERROR, ("decoded audio buffer %"GST_TIME_FORMAT,
> +        GST_TIME_ARGS(timestamp * GST_USECOND)));
> +
> +  AudioDataValue *data = new AudioDataValue[frames * mInfo.mAudioChannels];

Check against the possiblity of overflow. In particular some gcc
versions have a bug whereby operator new doesn't check for overflow if
the allocation count times the size of the object overflows. We use
PR_STATIC_ASSERT for this type of thing. See
content/media/wave/nsWaveReader.cpp for example which has:

+  PR_STATIC_ASSERT(PRUint64(BLOCK_SIZE) < UINT_MAX / sizeof(AudioDataValue) / MAX_CHANNELS);
+  const size_t bufferSize = static_cast<size_t>(frames * mChannels);
+  nsAutoArrayPtr<AudioDataValue> sampleBuffer(new AudioDataValue[bufferSize]);

Also use nsAutoArrayPtr here (as the wave example does).

@@ +238,5 @@
> +      frames, data, mInfo.mAudioChannels);
> +
> +  mAudioQueue.Push(audio);
> +
> +  gst_buffer_unref(buffer);

Is it worth creating some safe ref/unref class that unrefs in the
destructor for Gst objects?

@@ +244,5 @@
> +  return true;
> +}
> +
> +bool nsGStreamerReader::DecodeVideoFrame(bool &aKeyFrameSkip,
> +                                      PRInt64 aTimeThreshold)

Align PRInt64 with bool in the line above. Ditto with any similar
alignment in function arguments later in the file.

@@ +277,5 @@
> +    timestamp = gst_segment_to_stream_time(&mVideoSegment,
> +        GST_FORMAT_TIME, timestamp);
> +    timestamp = nextTimestamp = GST_TIME_AS_USECONDS(timestamp);
> +    if (GST_CLOCK_TIME_IS_VALID(GST_BUFFER_DURATION(buffer))) {
> +        nextTimestamp += GST_TIME_AS_USECONDS(GST_BUFFER_DURATION(buffer));

Is there danger of overflow here? Should it be checked?

@@ +324,5 @@
> +                                   nextTimestamp,
> +                                   b,
> +                                   isKeyframe,
> +                                   -1,
> +                                   mPicture);

Align with 'mInfo' in line 320.

@@ +587,5 @@
> +  GstCaps *caps = gst_pad_get_negotiated_caps(sinkpad);
> +  GstStructure *s = gst_caps_get_structure(caps, 0);
> +  gst_structure_get_int(s, "rate", (gint *) &mInfo.mAudioRate);
> +  gst_structure_get_int(s, "channels", (gint *) &mInfo.mAudioChannels);
> +  mInfo.mHasAudio = true;

Might need to sanity check these values to ensure they're within a valid
range (to prevent overflow's, etc later). Ditto with the video
parameters later.

::: content/media/gstreamer/nsGStreamerReader.h
@@ +73,5 @@
> +  }
> +
> +private:
> +  static void NewDecodedPadCb(GstElement *aDecodebin, GstPad *aPad,
> +    gboolean last, gpointer aUserData);

s/last/aLast

@@ +103,5 @@
> +  void NewAudioBuffer();
> +
> +  static void EosCb(GstAppSink *aSink, gpointer aUserData);
> +  void Eos(GstAppSink *aSink);
> +

Can you add comments to the functions above with a brief explanation of
when/why they're called.

@@ +121,5 @@
> +  GstAppSinkCallbacks mSinkCallbacks;
> +  mozilla::ReentrantMonitor mGstThreadsMonitor;
> +  GstSegment mVideoSegment;
> +  GstSegment mAudioSegment;
> +  bool mReachedEos;

Some comments for what these are used for, threading requirements (if
any), etc if possible.

::: js/src/ctypes/libffi/Makefile.am
@@ +82,4 @@
>  
>  MAKEOVERRIDES=
>  
> +ACLOCAL_AMFLAGS= -I m4

Was this included in the patch by mistake?

-- 
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla.
https://bugs.launchpad.net/bugs/412647

Title:
  Firefox is not able to play mp4 <video> tags

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/412647/+subscriptions