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 Ubuntu Bugs, which is subscribed to Ubuntu. 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 -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
