[Bug 412647]

Alessandro Decina alessandro.d at gmail.com
Tue Apr 3 04:52:59 UTC 2012


Thanks for the review. I'm attaching a new patch that addresses most of
the issues you pointed out (and more, see inline comments below). I also
started pushing my git repository to https://github.com/alessandrod
/mozilla-central which hopefully helps to understand what's changed.


(In reply to Chris Double (:doublec) from comment #261)
> 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

Done


> 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.

I started running the mochi tests in content/media/ and fixed a good
number of issues already. Not all the tests pass, but most of those that
don't pass fail because gstreamer is reporting slightly different
durations (the diff is in the order of milliseconds) from what the tests
expect.

I'm going to add h264 clips to the test suite now and make sure that all
the 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.

Eventually yeah, it might make sense to enable gst only for h264. I left
it enabled for everything for now though so that I can find and fix more
issues running all the existing tests.


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

Done


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

Done
 
> @@ +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?

Needs to be handled. I switched to using playbin2 instead of decodebin2
and added error handling for gst_element_factory_make.


> @@ +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?

Yes, fixed.


> @@ +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.

The channels={1,2} caps restriction on the audio sink takes care of this. So
either audio is non present, or channels is set to a meaningful value.


> @@ +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]);

I can't use PR_STATIC_ASSERT as the size of the buffer is not fixed. I
can avoid the multiplication though and did so.

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

Done

 
> @@ +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?

I don't think so. The code that does gst refcounting is self contained
and it's done just in an handful of places. Don't have a strong
opinion/preference on this though.


 
> @@ +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.

Done


> 
> @@ +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?

Here timestamp is something that goes (roughly) from zero for the first
frame, to the duration of the stream for the last, so in normal
circumstances this won't overflow.

It can overflow I guess if GST_BUFFER_DURATION is specifically forged.
I'm going to see if I can create a test about this.


> @@ +324,5 @@
> > +                                   nextTimestamp,
> > +                                   b,
> > +                                   isKeyframe,
> > +                                   -1,
> > +                                   mPicture);
> 
> Align with 'mInfo' in line 320.

Done


> 
> @@ +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.

Gst already does as we have caps restrictions on the sinks. I added some
NS_ASSERTIONs in addition.


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

Done. If needed, I guess I can add more general comments about how
threading synchronization works between gst threads and the decoder
state machine threads.


> ::: js/src/ctypes/libffi/Makefile.am
> @@ +82,4 @@
> >  
> >  MAKEOVERRIDES=
> >  
> > +ACLOCAL_AMFLAGS= -I m4
> 
> Was this included in the patch by mistake?

Oops, yes.

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




More information about the Ubuntu-mozillateam-bugs mailing list