Closed Bug 855130 Opened 11 years ago Closed 11 years ago

Implement enough of the Media Source Extensions API to run the demo app

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: kinetik, Assigned: kinetik)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(6 files, 18 obsolete files)

3.26 KB, patch
khuey
: feedback+
kinetik
: checkin+
Details | Diff | Splinter Review
49.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.17 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
274.87 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
16.71 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
19.17 KB, patch
khuey
: review+
Details | Diff | Splinter Review
Implement enough of the Media Source Extensions API so that the following works:

  var ms = new MediaSource();

  var video = document.querySelector('video');
  video.src = window.URL.createObjectURL(ms);

  ms.addEventListener('sourceopen', function(e) {
    ...
    var sourceBuffer = ms.addSourceBuffer('video/webm; codecs="vorbis,vp8"');
    sourceBuffer.append(oneVideoWebMChunk);
    ...
  }, false);

The bug URL links to a demo app that uses this to play a video.  It'll need slight modifications to work in Firefox, as it relies on the events including a "webkit" prefix.
Attached patch wip (applies to aed1da04448d) (obsolete) — Splinter Review
This works with the slightly modified version of the demo hosted here: http://flim.org/~kinetik/mediasource_demo.html (changes: rename append to appendBuffer per current spec, removed webkit prefix from events).

Still to do: fix leaks and a shutdown hang.
Windows build has problems with compiling content/media/mediasource/MediaSourceInputAdapter.h and .cpp: nsresult must be changed to NS_IMETHOD or NS_IMETHODIMP, e.g.

+  NS_IMETHOD Close();
...
+
+NS_IMETHODIMP
+MediaSourceInputAdapter::Close()
+{
Attached patch wip v2 (applies to aed1da04448d) (obsolete) — Splinter Review
Fix nsresult/NS_IMETHODIMP compile error on Win32 and switch InputAdapter from file to (unseekable) channel interface which allows multi-append playback to work with more decoders.
Attachment #729909 - Attachment is obsolete: true
I found some warnings-as-errors build issues with wip-v2: https://tbpl.mozilla.org/?tree=Try&rev=2a9e15d21a29.

This patch gives a clean try build: https://tbpl.mozilla.org/?tree=Try&rev=eb821c1c3b16.(I also noticed that mochitest-1 often hits that shutdown hang.)
We are seeing some intermittent problems with the v2 patch when playing http://flim.org/~kinetik/mediasource_demo.html: On desktop, it usually almost completes, but not quite; on mobile devices, it sometimes stops quite soon, like at 1:03, and other times nearly completes.
Attached patch wip v3 (applies to 7b8ed29c6bc0) (obsolete) — Splinter Review
Rebased, integrated gbrown's warning fixes, possibly fixed the stalling bug (bogus blocking calculation in Read()).
Attachment #736095 - Attachment is obsolete: true
Attachment #736286 - Attachment is obsolete: true
The stalling bug I reported in Comment 5 seems to be resolved by wip-v3 -- it works great now!
On FF/Mac with mediasource+gstreamer enabled, the web page with MediaSource and MP4 file locks the browser, try http://async5.org/moz/msmp4.html .
(FF/Mac/gstreamer) fragmented MP4 does not start playback until endOfStream is called, see example at http://async5.org/moz/story.html
I feel like I must be missing something obvious here, but I've got a leak that I can't work out and need some expertise to rescue me.  Attached is a minimized patch that implements MediaSource and a createObjectURL overload.  The included testcase, test_MediaSource.html, works correctly without the following line:

  ms.foo = null;

But with that line added, a whole lot of stuff leaks, including the MediaSource instance.  Am I missing some CC macro magic?
Attachment #744478 - Flags: feedback?(bzbarsky)
Comment on attachment 744478 [details] [diff] [review]
minimal patch for leak debugging v0

Hrm.  The CC macros look correct for a wrappercached object, so that part is OK.

I think you may be ending up with a cycle like so:

  MediaSource -> JSObject (preserved wrapper) -> Window's JSObject -> Document ->
    nsHostObjectProtocolHandler data entry -> MediaSource

At least I don't see anything obvious that would break this cycle... Kyle, do you know about this data entry stuff, or know who does?
Attachment #744478 - Flags: feedback?(bzbarsky) → feedback?(khuey)
(In reply to Boris Zbarsky (:bz) from comment #12)
> I think you may be ending up with a cycle like so:
> 
>   MediaSource -> JSObject (preserved wrapper) -> Window's JSObject ->
> Document ->
>     nsHostObjectProtocolHandler data entry -> MediaSource

Yes, I agree that that is what is likely happening.

> At least I don't see anything obvious that would break this cycle... Kyle,
> do you know about this data entry stuff, or know who does?

There is nothing that will break this cycle.  The existing consumer of this data entry stuff is the blob code.  Blobs don't preserve wrappers so this cycle is not possible.  The existing teardown code is located at http://hg.mozilla.org/mozilla-central/annotate/e474b6cfebce/content/base/src/nsDocument.cpp#l1501 in ~nsDocument, but obviously that won't help if you have a cycle.

So it sounds like we need to traverse through the protocol handler.
Matthew, are you OK giving that a shot?
I can find someone to do that if necessary.
Thanks for the explanations.  I'm not really sure what I'm doing here, but this seems to work.
Attachment #744941 - Flags: feedback?(khuey)
Attachment #744941 - Attachment description: patch v0 → traverse protocolhandler patch v0
Attachment #744478 - Attachment is obsolete: true
14:36 < khuey|tw> kinetik: fwiw you can treat that f+ as an r+ if you like
Simple fix: Traverse needs to null-check gDataTable.
Whiteboard: [leave open]
Attachment #744941 - Flags: checkin+
Attached patch wip v4 (applies to 7e3a4ebcf067) (obsolete) — Splinter Review
There's a lot of implementation work left to do, but this patch meets the goals set out in this bug and the rest of the work will hang off of bug 778617.  Posting for early feedback, I've got a couple more tests to add and some very minor cleanups to do in parallel, then I'd like to get this reviewed and landed.
Attachment #757783 - Flags: feedback?(Ms2ger)
(In reply to Yury Delendik (:yury) from comment #9)
> On FF/Mac with mediasource+gstreamer enabled, the web page with MediaSource
> and MP4 file locks the browser, try http://async5.org/moz/msmp4.html .

This should be fixed now.

(In reply to Yury Delendik (:yury) from comment #10)
> (FF/Mac/gstreamer) fragmented MP4 does not start playback until endOfStream
> is called, see example at http://async5.org/moz/story.html

Fragmented MP4 support will happen in a separate bug, it's quite a large chunk of work.
Does this have a spec?

I'd also appreciate a diff with 8 lines of context, if possible.
Attachment #736650 - Attachment is obsolete: true
Sorry, updated with 8 lines of context.

The spec is hosted here: http://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html

Note that the current patch is based on the May 13th version; I'll update to June 1st before final review/landing.
Attachment #757783 - Attachment is obsolete: true
Attachment #757783 - Flags: feedback?(Ms2ger)
Attachment #758422 - Flags: feedback?(Ms2ger)
Comment on attachment 758422 [details] [diff] [review]
wip v4.1 (applies to 22cb668fd727)

Review of attachment 758422 [details] [diff] [review]:
-----------------------------------------------------------------

Random comments from a first reading; I haven't compared with the spec for now.

::: content/base/public/nsHostObjectProtocolHandler.h
@@ +64,5 @@
>  
> +class nsMediaSourceProtocolHandler : public nsHostObjectProtocolHandler
> +{
> +public:
> +  NS_IMETHOD GetScheme(nsACString &result);

moz_override

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +15,5 @@
>  // -----------------------------------------------------------------------
>  // Hash table
>  struct DataInfo
>  {
> +  // mObject must be an nsIDOMBlob, nsIDOMMediaStream, or MediaSource

I'm not sure what "must be" means here

@@ +214,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsISupports> owner = do_QueryInterface(info->mPrincipal);
>  
> +  nsAutoString type;

Why nsAutoString? There's a good chance nsString could avoid the copy by refcounting the buffer, aiui

::: content/html/content/public/HTMLMediaElement.h
@@ +30,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/dom/TextTrack.h"
>  #include "mozilla/dom/TextTrackList.h"
>  #include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/MediaPlaybackQuality.h"

Goes between mozilla/Attributes.h and mozilla/dom/TextTrack.h.

@@ -32,5 @@
>  #include "mozilla/dom/TextTrackList.h"
>  #include "mozilla/ErrorResult.h"
> -
> -// Define to output information on decoding and painting framerate
> -/* #define DEBUG_FRAME_RATE 1 */

Unrelated change?

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1108,5 @@
>      SetupSrcMediaStreamPlayback(static_cast<DOMMediaStream*>(stream.get()));
>      return NS_OK;
>    }
>  
> +  // XXX(kinetik): wire this up properly.

Bug number, if you don't fix it in this bug

@@ +1110,5 @@
>    }
>  
> +  // XXX(kinetik): wire this up properly.
> +  if (IsMediaSourceURI(mLoadingSrc)) {
> +    nsCOMPtr<MediaSource> source;

nsRefPtr

@@ +1120,5 @@
> +      const PRUnichar* params[] = { spec.get() };
> +      ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
> +      return rv;
> +    }
> +    mMediaSource = source.get();

Shouldn't need the .get(). (But you could use forget() to avoid and addref/release pair.)

@@ +1122,5 @@
> +      return rv;
> +    }
> +    mMediaSource = source.get();
> +    mMediaSource->AttachElement(this);
> +    // XXX(kinetik): bail here rather than relying on mediasource->channel conversion.

Ditto

@@ +3619,5 @@
> +  }
> +  nsPerformance* perf = window->GetPerformance();
> +  if (!perf) {
> +    return nullptr;
> +  }

NS_ENSURE_TRUE for both of those.

::: content/media/MediaPlaybackQuality.cpp
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "mozilla/dom/HTMLMediaElement.h"
> +#include "MediaPlaybackQuality.h"

Include this one first, and leave an empty line before and after.

@@ +15,5 @@
> +                                           DOMHighResTimeStamp aCreationTime,
> +                                           uint64_t aTotalFrames,
> +                                           uint64_t aDroppedFrames)
> +  : mElement(aElement), mCreationTime(aCreationTime),
> +    mTotalFrames(aTotalFrames), mDroppedFrames(aDroppedFrames)

One line each, and , lined up under :.

@@ +45,5 @@
> +  return mTotalFrames;
> +}
> +
> +uint64_t
> +MediaPlaybackQuality::DroppedVideoFrames()

These three can be inlined and made const

::: content/media/MediaPlaybackQuality.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Copy the boilerplate from <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line> for all new files. (Including 'file,' on the third line, tab-width: 8 in the emacs modeline and the vim modeline.)

@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +#define MOZILLA_MEDIAPLAYBACKQUALITY_H_

mozilla_dom_MediaPlaybackQuality_h

@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +#define MOZILLA_MEDIAPLAYBACKQUALITY_H_
> +
> +#include "mozilla/dom/MediaPlaybackQualityBinding.h"

Don't include this in the header, but in the .cpp

@@ +13,5 @@
> +namespace dom {
> +
> +class HTMLMediaElement;
> +
> +class MediaPlaybackQuality MOZ_FINAL : public nsISupports, public nsWrapperCache

", public nsWrapperCache" on its own line, with , lined up with :

::: content/media/mediasource/MediaSource.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "MediaSource.h"

Empty line before and after this

@@ +18,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +nsresult
> +MediaSource::GetType(nsAString& aType)

Why return nsresult?

@@ +25,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +MediaSource::GetInternalStream(nsIInputStream** aStream)

Ditto. This can return the stream directly.

@@ +33,5 @@
> +  adapter.forget(aStream);
> +  return NS_OK;
> +}
> +
> +/*static*/ already_AddRefed<MediaSource>

Nit: /* static */

@@ +50,5 @@
> +already_AddRefed<SourceBufferList>
> +MediaSource::SourceBuffers()
> +{
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);

MOZ_ASSERT_IF(mReadyState == dom::ReadyState::Closed, mSourceBuffers->Length() == 0);

(You could also add IsEmpty().)

@@ +53,5 @@
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);
> +  }
> +  nsRefPtr<SourceBufferList> list = mSourceBuffers;
> +  return list.forget();

No reason for the AddRef here. Just return a raw pointer.

@@ +63,5 @@
> +  if (mReadyState == dom::ReadyState::Closed) {
> +    MOZ_ASSERT(mSourceBuffers->Length() == 0);
> +  }
> +  nsRefPtr<SourceBufferList> list = mActiveSourceBuffers;
> +  return list.forget();

Ditto (×2)

@@ +96,5 @@
> +  }
> +  DurationChange(aDuration, aRv);
> +  if (aRv.Failed()) {
> +    return;
> +  }

That's hardly useful at the end of the function

@@ +107,5 @@
> +    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +    return nullptr;
> +  }
> +  // Check aType against HTMLMediaElement list of MIME types.
> +  // XXX: Further restrict this to formats in the spec.

Fix or file a bug (and note the number)

@@ +112,5 @@
> +  if (HTMLMediaElement::GetCanPlay(aType) == CANPLAY_NO) {
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return nullptr;
> +  }
> +  // XXX: temporary limit.

Ditto

@@ +138,5 @@
> +    aRv.Throw(NS_ERROR_DOM_NOT_FOUND_ERR);
> +    return;
> +  }
> +  // If aSourceBuffer is updating:
> +  if (aSourceBuffer.Updating()) {

You're inconsistent: you use both sourceBuffer and aSourceBuffer

@@ +162,5 @@
> +  }
> +  // Remove aSourceBuffer from sourceBuffers and fire "removesourcebuffer" at sourceBuffers
> +  mSourceBuffers->Remove(sourceBuffer);
> +  aSourceBuffer.Detach();
> +  // Free all resources associated with aSourceBuffer

I assume all these comments will be filled in later.

@@ +180,5 @@
> +/*static*/ bool
> +MediaSource::IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType)
> +{
> +  if (aType.IsEmpty()) {
> +    return false;

Interesting. Can't HTMLMediaElement::GetCanPlay deal with empty strings?

@@ +196,5 @@
> +  NotifyListeners();
> +}
> +
> +void
> +MediaSource::AttachElement(HTMLMediaElement* aElement)

(This isn't called yet?)

@@ +201,5 @@
> +{
> +  LOG(PR_LOG_DEBUG, ("%p Attaching element %p", this, aElement));
> +  MOZ_ASSERT(aElement);
> +  mElement = aElement;
> +  if (mReadyState != dom::ReadyState::Closed) {

Btw, you don't need the dom:: anywhere.

@@ +232,5 @@
> +}
> +
> +MediaSource::MediaSource(nsPIDOMWindow* aWindow)
> +  : mElement(nullptr),
> +    mDuration(UnspecifiedNaN()),

, to the front

@@ +237,5 @@
> +    mMonitor("mozilla::dom::MediaSource::mMonitor"),
> +    mReadyState(dom::ReadyState::Closed)
> +{
> +  mSourceBuffers = new SourceBufferList(this);
> +  mActiveSourceBuffers = new SourceBufferList(this);

Why not use initializers for those?

@@ +247,5 @@
> +#endif
> +
> +  MOZ_ASSERT(aWindow);
> +  nsDOMEventTargetHelper::BindToOwner(aWindow);
> +  SetIsDOMBinding();

You can use the nsDETH constructor that takes a window instead.

@@ +387,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(MediaSource)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEventTargetHelper)
> +
> +}
> +}

"} // namespace foo" (and anywhere else I missed it)

::: content/media/mediasource/MediaSource.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_MEDIASOURCE_H_
> +#define MOZILLA_MEDIASOURCE_H_

mozilla_dom_MediaSource_h

@@ +6,5 @@
> +#define MOZILLA_MEDIASOURCE_H_
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/Monitor.h"
> +#include "mozilla/dom/HTMLMediaElement.h"

Check if you need this include

@@ +23,5 @@
> +class SourceBufferList;
> +class SourceBuffer;
> +
> +template <typename T>
> +class AsyncEventRunnner : public nsRunnable

Does this need to be templated?

@@ +28,5 @@
> +{
> +public:
> +  AsyncEventRunnner(T* aTarget, const nsAString& aName)
> +    : mTarget(aTarget),
> +      mName(aName)

, to the front

@@ +31,5 @@
> +    : mTarget(aTarget),
> +      mName(aName)
> +  {}
> +
> +  NS_IMETHOD Run() {

{ on its own line

@@ +71,5 @@
> +  static bool IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType);
> +
> +  void AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv);
> +
> +  // Semi-private, for MSIA only.

MSIA?

@@ +74,5 @@
> +
> +  // Semi-private, for MSIA only.
> +  nsTArray<uint8_t> const& GetData() { return mData; }
> +  Monitor& GetMonitor() { return mMonitor; }
> +  bool AppendDone() { return mReadyState == dom::ReadyState::Closed; }

Please use the 4-line form

@@ +97,5 @@
> +  void DurationChange(double aNewDuration, ErrorResult& aRv);
> +  void EndOfStreamInternal(const Optional<EndOfStreamError>& aError, ErrorResult& aRv);
> +
> +
> +  HTMLMediaElement* mElement;

What's the ownership model here? Document.

@@ +112,5 @@
> +
> +  nsTArray<nsRefPtr<MediaSourceInputAdapter> > mAdapters;
> +
> +  // Protected by monitor.
> +  nsTArray<uint8_t> mData;

Consider the ordering of these members; biggest-to-smallest tends to waste the least.

::: content/media/mediasource/MediaSourceInputAdapter.cpp
@@ +27,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::Available(uint64_t* _retval)

aRetval or aAvailable

@@ +40,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::Read(char* aBuf, uint32_t aCount, uint32_t* _retval)

Ditto

@@ +47,5 @@
> +}
> +
> +NS_IMETHODIMP
> +MediaSourceInputAdapter::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
> +                                      uint32_t aCount, uint32_t* _retval)

Ditto

@@ +62,5 @@
> +  }
> +
> +  uint32_t count = std::min(aCount, available);
> +  nsresult rv = aWriter(this, aClosure,
> +                        reinterpret_cast<const char*>(mMediaSource->GetData().Elements() + mOffset),

Can this be &mMediaSource->GetData()[mOffset]?

@@ +116,5 @@
> +void
> +MediaSourceInputAdapter::NotifyListener()
> +{
> +  if (mCallback && (mMediaSource->GetData().Length() - mOffset >= mNotifyThreshold ||
> +                    mMediaSource->AppendDone() || mClosed)) {

Early return. And maybe a note what the second half means; it's not immediately obvious.

@@ +139,5 @@
> +}
> +
> +MediaSourceInputAdapter::MediaSourceInputAdapter(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource),
> +    mOffset(0),

Commas

@@ +154,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MediaSourceInputAdapter)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +  NS_INTERFACE_MAP_ENTRY(nsIInputStream)
> +  NS_INTERFACE_MAP_ENTRY(nsIAsyncInputStream)
> +  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(MediaSourceInputAdapter)

This should be included in NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION. (But double-check.)

::: content/media/mediasource/MediaSourceInputAdapter.h
@@ +31,5 @@
> +  uint32_t mNotifyThreshold;
> +
> +  nsRefPtr<MediaSource> mMediaSource;
> +  int64_t mOffset;
> +  bool mClosed;

These take 6 words on x64, but could fit in 5.

::: content/media/mediasource/SourceBuffer.cpp
@@ +44,5 @@
> +
> +double
> +SourceBuffer::TimestampOffset()
> +{
> +  return mTimestampOffset;

const and inline?

@@ +118,5 @@
> +
> +void
> +SourceBuffer::AppendBuffer(ArrayBuffer& aData, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(sizeof(*aData.Data()) == 1);

Given that you're passing this to a function that takes uint8_t*, I think you're fine.

@@ +184,5 @@
> +}
> +
> +SourceBuffer::SourceBuffer(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource),
> +    mAppendMode(AppendMode::Segments),

Commas

@@ +192,5 @@
> +    mTimestampOffset(0),
> +    mAttached(false)
> +{
> +  MOZ_ASSERT(aMediaSource);
> +  nsDOMEventTargetHelper::BindToOwner(aMediaSource->GetParentObject());

Constructor

::: content/media/mediasource/SourceBuffer.h
@@ +82,5 @@
> +  double mAppendWindowEnd;
> +
> +  double mTimestampOffset;
> +
> +  bool mAttached;

The usual

::: content/media/mediasource/SourceBufferList.cpp
@@ +20,5 @@
> +{
> +  if (aIndex < mSourceBuffers.Length()) {
> +    aFound = true;
> +    nsRefPtr<SourceBuffer> buf = mSourceBuffers[aIndex];
> +    return buf.forget();

I see no reason for the addref

@@ +33,5 @@
> +  return mSourceBuffers.Length();
> +}
> +
> +void
> +SourceBufferList::Append(nsRefPtr<SourceBuffer> aSourceBuffer)

nsRefPtr arguments? Why?

@@ +43,5 @@
> +void
> +SourceBufferList::Remove(nsRefPtr<SourceBuffer> aSourceBuffer)
> +{
> +  DebugOnly<bool> removed = mSourceBuffers.RemoveElement(aSourceBuffer);
> +  MOZ_ASSERT(removed);

Could also use MOZ_ALWAYS_TRUE

@@ +112,5 @@
> +SourceBufferList::SourceBufferList(MediaSource* aMediaSource)
> +  : mMediaSource(aMediaSource)
> +{
> +  MOZ_ASSERT(aMediaSource);
> +  nsDOMEventTargetHelper::BindToOwner(aMediaSource->GetParentObject());

Constructor

::: content/media/mediasource/SourceBufferList.h
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef MOZILLA_SOURCEBUFFERLIST_H_
> +#define MOZILLA_SOURCEBUFFERLIST_H_

As usual

@@ +7,5 @@
> +
> +#include "MediaSource.h"
> +#include "SourceBuffer.h"
> +#include "mozilla/Attributes.h"
> +#include "mozilla/dom/SourceBufferListBinding.h"

Do you need this?

::: content/media/mediasource/moz.build
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PARALLEL_DIRS += ['test']

I don't think we're entirely consistent yet, but

PARALLEL_DIRS += [
    'test'
]

::: content/media/mediasource/test/Makefile.in
@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES = \
> +		test_MediaSource.html \
> +		$(NULL)

Indent by two spaces

::: content/media/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +PARALLEL_DIRS += ['mediasource', 'webaudio']

PARALLEL_DIRS += [
    'mediasource',
    'webaudio'
]

::: dom/bindings/Bindings.conf
@@ +598,5 @@
>  
> +'MediaSource': [{
> +},
> +{
> +    'nativeType': 'JSObject',

Gah.

::: dom/webidl/MediaSource.webidl
@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +enum ReadyState {

This exposes a dom::ReadyState... Please call it something more specific.

@@ +25,5 @@
> +interface MediaSource : EventTarget {
> +  readonly attribute SourceBufferList sourceBuffers;
> +  readonly attribute SourceBufferList activeSourceBuffers;
> +  readonly attribute ReadyState readyState;
> +  [SetterThrows] attribute unrestricted double duration;

All the Mozilla-specific extended attributes go on their own line.

::: layout/build/nsLayoutModule.cpp
@@ +272,5 @@
>  NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsDOMFileReader, Init)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsFormData)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsBlobProtocolHandler)
>  NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaStreamProtocolHandler)
> +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)

Do we need all this?
Attachment #758422 - Flags: feedback?(Ms2ger)
(In reply to :Ms2ger from comment #27)
> I assume all these comments will be filled in later.

In a later bug, yep.

> (This isn't called yet?)

Called from HTMLMediaElement.

> Btw, you don't need the dom:: anywhere.

It's required to disambiguate the ReadyState enum from ReadyState().  Once I rename the ReadyState enum as requested below it won't be necessary.

> Why not use initializers for those?

I did originally, it causes warnings.  See comment 4.

> Does this need to be templated?

Not without introducing a common interface for the users.  If I removed the logging from DispatchSimpleEvent I could remove the templating and call DispatchTrustedEvent (common on nsDETH) directly.

> Gah.

Required for the MediaSource overload of URL::createObjectURL. :-(

> This exposes a dom::ReadyState... Please call it something more specific.

MediaSourceReadyState?

Does the same apply for the other new enums (AppendMode, EndOfStreamError)?

> > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> 
> Do we need all this?

Not sure how to find out the answer to this.
(In reply to Matthew Gregan [:kinetik] from comment #28)
> > This exposes a dom::ReadyState... Please call it something more specific.
> 
> MediaSourceReadyState?

Yeah!

> Does the same apply for the other new enums (AppendMode, EndOfStreamError)?

I think so.

> > > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> > 
> > Do we need all this?
> 
> Not sure how to find out the answer to this.

Maybe we don't, but just put it in for now.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> (In reply to Matthew Gregan [:kinetik] from comment #28)
> > > This exposes a dom::ReadyState... Please call it something more specific.
> > 
> > MediaSourceReadyState?
> 
> Yeah!

Sounds good to me too.

> > Does the same apply for the other new enums (AppendMode, EndOfStreamError)?
> 
> I think so.

I feel less strongly about those; these names aren't overloaded so heavily.

> > > > +NS_GENERIC_FACTORY_CONSTRUCTOR(nsMediaSourceProtocolHandler)
> > > 
> > > Do we need all this?
> > 
> > Not sure how to find out the answer to this.
> 
> Maybe we don't, but just put it in for now.

wfm
Thanks for the feedback!  This patch addresses everything that I didn't cover in comment 28.
Attachment #758422 - Attachment is obsolete: true
Attachment #761288 - Flags: review?(Ms2ger)
Same, but rebased to current tip (minor changes to content/media/moz.build).
Attachment #761288 - Attachment is obsolete: true
Attachment #761288 - Flags: review?(Ms2ger)
Attachment #761295 - Flags: review?(Ms2ger)
(I can take another look, probably some time next week, but I should mention that I'm probably not going to feel comfortable granting r+.)
Should I convert the request to a feedback? and maybe add Boris for review?
If you break out the host-object stuff into its own patch, khuey can review that.

I think I can review the rest. I did a lot of Web Audio reviews which is similar in scope to this code.
Attachment #761295 - Attachment is obsolete: true
Attachment #761295 - Flags: review?(Ms2ger)
Split HostObjectProtocolHandler and URL changes out.  Improved mochitest to cover all of the initial functionality this bug targets.
Attachment #763990 - Flags: review?(roc)
Applies on top of main patch.  Add MediaSource to HostObjectProtocolHandler and extend URL::createObjectURL to support MediaSource.
Attachment #763991 - Flags: review?(khuey)
Comment on attachment 763990 [details] [diff] [review]
patch v4.4 (applies to 1790f40f71f0)

Obsoleting these patches and splitting into smaller chunks.
Attachment #763990 - Attachment is obsolete: true
Attachment #763990 - Flags: review?(roc)
Attachment #763991 - Attachment is obsolete: true
Attachment #763991 - Flags: review?(khuey)
Attached patch MSE p1 VideoPlaybackQuality v1.0 (obsolete) — Splinter Review
Attachment #764582 - Flags: review?(roc)
Attached patch MSE p2 MediaSource v1.0 (obsolete) — Splinter Review
Attachment #764583 - Flags: review?(roc)
Attachment #764585 - Flags: review?(roc)
Attached patch MSE p5 MSE tests v1.0 (obsolete) — Splinter Review
Attachment #764586 - Flags: review?(roc)
Comment on attachment 764582 [details] [diff] [review]
MSE p1 VideoPlaybackQuality v1.0

Review of attachment 764582 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLVideoElement.cpp
@@ +264,5 @@
> +    corruptedFrames = totalFrames - stats.GetDecodedFrames();
> +  }
> +  VideoFrameContainer* container = GetVideoFrameContainer();
> +  if (container) {
> +    playbackJitter = container->GetFrameDelay();

The spec says that this should be the sum of the absolute values of the delay for each frame. This is only the delay for the last frame (and no absolute values in sight).

::: content/media/VideoPlaybackQuality.h
@@ +15,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class VideoPlaybackQuality MOZ_FINAL : public nsISupports
> +                                     , public nsWrapperCache

I think this doesn't need to inherit from nsISupports. See http://lxr.mozilla.org/mozilla-central/source/dom/encoding/TextDecoder.h for an example of how to do this without nsISupports. But let me check with someone to find out when we are/aren't supposed to use nsISupports with WebIDL.
Comment on attachment 764582 [details] [diff] [review]
MSE p1 VideoPlaybackQuality v1.0

Review of attachment 764582 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLVideoElement.cpp
@@ +247,5 @@
> +already_AddRefed<dom::VideoPlaybackQuality>
> +HTMLVideoElement::VideoPlaybackQuality()
> +{
> +  nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> +  NS_ENSURE_TRUE(window, nullptr);

This attribute isn't nullable in the IDL, so this will crash. (Probably not exploitable anymore, but who knows...)

@@ +249,5 @@
> +{
> +  nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow();
> +  NS_ENSURE_TRUE(window, nullptr);
> +  nsPerformance* perf = window->GetPerformance();
> +  NS_ENSURE_TRUE(perf, nullptr);

Another crash.

::: content/media/VideoPlaybackQuality.h
@@ +15,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +class VideoPlaybackQuality MOZ_FINAL : public nsISupports
> +                                     , public nsWrapperCache

I don't see a reason why this should inherit from nsISupports, on first sight. And if JS can only ever get hold of newly created objects, it can probably be an owned class. (Might even be able to drop the refcounting.)

::: dom/webidl/HTMLVideoElement.webidl
@@ +45,5 @@
>    // True if the video has an audio track available.
>    readonly attribute boolean mozHasAudio;
>  };
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-VideoPlaybackQuality

https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-HTMLVideoElement

@@ +48,5 @@
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-VideoPlaybackQuality
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled"]
> +  readonly attribute VideoPlaybackQuality videoPlaybackQuality;

This always returns a new object? Add [Creator], and complain about the spec, because something that returns a new object every time should be a method, not an attribute.

@@ +50,5 @@
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled"]
> +  readonly attribute VideoPlaybackQuality videoPlaybackQuality;
> +};
> + 

Trailing whitespace
Comment on attachment 764583 [details] [diff] [review]
MSE p2 MediaSource v1.0

Review of attachment 764583 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks fine.

One big spec issue I realized here: I think appendBuffer() should neuter the array parameter and take ownership of the underlying data buffer. This would reduce copies and memory usage, potentially by a lot. Does that make sense? If so, please bring it up on the list ASAP.

::: content/media/mediasource/MediaSource.cpp
@@ +24,5 @@
> +
> +nsString
> +MediaSource::GetType()
> +{
> +  return mContentType;

Might as well be inline in the header. Also I think we can return const nsString& here?

@@ +28,5 @@
> +  return mContentType;
> +}
> +
> +already_AddRefed<nsIInputStream>
> +MediaSource::GetInternalStream()

Better to call this CreateInternalStream?

@@ +99,5 @@
> +  if (!IsTypeSupportedInternal(aType, aRv)) {
> +    return nullptr;
> +  }
> +  // XXX(kinetik): Bug 881512. Temporary limit until multiple decoders are supported.
> +  if (mSourceBuffers->Length() == 1) {

Make this >= 1 ?

@@ +183,5 @@
> +MediaSource::AttachElement(HTMLMediaElement* aElement)
> +{
> +  LOG(PR_LOG_DEBUG, ("%p Attaching element %p", this, aElement));
> +  MOZ_ASSERT(aElement);
> +  mElement = aElement;

Does the spec explicitly say anywhere that there can be only one one element attached at a time? I don't see that it does. That should be fixed in the spec, and it will have to say what happens when someone tries to attach the same MediaSource URI to multiple media elements. I think at least here we should have AttachElement return false and fail if it can't attach, and have the media element try to recover from that.

We have to be careful that since mElement is weak, we don't allow applications to detect the GC of the HTMLMediaElement while the MediaSource remains alive. It might be better to make mElement an nsRefPtr and use CC here.

@@ +326,5 @@
> +    // DurationChange(highestDurationOfSourceBuffers, aRv);
> +    // if (aRv.Failed()) {
> +    //   return;
> +    // }
> +    // Notify media element that all data is now available.

Sometimes it's not 100% clear that a comment is a TODO. I suggest explicitly adding TODO everywhere.

::: content/media/mediasource/MediaSource.h
@@ +29,5 @@
> +template <typename T>
> +class AsyncEventRunnner : public nsRunnable
> +{
> +public:
> +  AsyncEventRunnner(T* aTarget, const nsAString& aName)

This really belongs in some other file, but I guess it can stay here for now.

@@ +42,5 @@
> +  }
> +
> +private:
> +  nsRefPtr<T> mTarget;
> +  nsString mName;

How about just const char*, since these are always ASCII string literals? Pass const char* to DispatchSimpleEvent too.

@@ +73,5 @@
> +
> +  void EndOfStream(const Optional<MediaSourceEndOfStreamError>& aError, ErrorResult& aRv);
> +  static bool IsTypeSupported(const GlobalObject& aGlobal, const nsAString& aType);
> +
> +  void AppendData(const uint8_t* aData, uint32_t aLength, ErrorResult& aRv);

Use comments to delimit the WebIDL methods from the rest of the class.

@@ +94,5 @@
> +
> +  void AttachElement(HTMLMediaElement* aElement);
> +  void DetachElement();
> +
> +  explicit MediaSource(nsPIDOMWindow* aWindow);

Make this protected/private?

@@ +120,5 @@
> +
> +  // Protected by monitor.
> +  nsTArray<uint8_t> mData;
> +
> +  Monitor mMonitor;

Add comments explaining what mMonitor protects.
Comment on attachment 764585 [details] [diff] [review]
MSE p4 MediaElement integration v1.0

Review of attachment 764585 [details] [diff] [review]:
-----------------------------------------------------------------

I think it would make sense to support mozSrcObject = mediaSource, but that can be done separately.
Attachment #764585 - Flags: review?(roc) → review+
Comment on attachment 764586 [details] [diff] [review]
MSE p5 MSE tests v1.0

Review of attachment 764586 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice!
Attachment #764586 - Flags: review?(roc) → review+
> Add [Creator], and complain about the spec

You can't add [Creator] on an attribute, because that's nonsense.

Returning a new object from this thing each time is just not ok.  We need to either return a live object or have this be a method.
I think this should probably be a live object.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> I think it would make sense to support mozSrcObject = mediaSource, but that
> can be done separately.

Bug 886194.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #46)
> One big spec issue I realized here: I think appendBuffer() should neuter the
> array parameter and take ownership of the underlying data buffer. This would
> reduce copies and memory usage, potentially by a lot. Does that make sense?
> If so, please bring it up on the list ASAP.

w3.org Bug 22432.

> Does the spec explicitly say anywhere that there can be only one one element
> attached at a time? I don't see that it does. That should be fixed in the
> spec, and it will have to say what happens when someone tries to attach the
> same MediaSource URI to multiple media elements. I think at least here we
> should have AttachElement return false and fail if it can't attach, and have
> the media element try to recover from that.

w3.org Bug 22430.

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #49)
> Returning a new object from this thing each time is just not ok.  We need to
> either return a live object or have this be a method.

w3.org Bug 22431.
Comment on attachment 764584 [details] [diff] [review]
MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0

Review of attachment 764584 [details] [diff] [review]:
-----------------------------------------------------------------

I'm surprised this compiles.  There's a worker thread implementation of URL, how did you get away without modifying that?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #53)
> Comment on attachment 764584 [details] [diff] [review]
> MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0
> 
> Review of attachment 764584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm surprised this compiles.  There's a worker thread implementation of URL,
> how did you get away without modifying that?

It hits the JSObject& overload, I suppose.
Bindings.conf:

'MediaSource': [{
    'resultNotAddRefed': [ 'sourceBuffers', 'activeSourceBuffers' ],
},
{
    'nativeType': 'JSObject',
    'workers': True,
    'skipGen': True
}],

...so, yeah.  I copied DOMMediaStream, so presumably that's broken for the worker thread implementation of URL too?
Attachment #764585 - Attachment description: MSE p4 MediaElement integration integration v1.0 → MSE p4 MediaElement integration v1.0
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #49)
> > Add [Creator], and complain about the spec
> 
> You can't add [Creator] on an attribute, because that's nonsense.
> 
> Returning a new object from this thing each time is just not ok.  We need to
> either return a live object or have this be a method.

https://www.w3.org/Bugs/Public/show_bug.cgi?id=22431 points out that HTMLMediaElement.buffered/played/seekable are already using [Creator]...
Huh.  I thought we disallowed that!  We should.
We'd need a spec change, since they are currently specced to return fresh objects. See
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-buffered
Can you point this out on WHATWG and explain why this is bogus? Because I don't fully understand the problem.
We don't need a spec change to disallow [Creator] on attributes...  I did file https://www.w3.org/Bugs/Public/show_bug.cgi?id=22471 earlier today, though.
Oh, we can remove [Creator] but still return a new object every time? Great, let's do that here then :-).
[Creator] just tells binding code that you promise to return a new object every time, so we can enforce that you're not returning non-wrappercached objects unless you make that promise.  You can, of course, not promise but still do.  ;)

But my real concern here is that this API seems like a bad API from a JS developer perspective, and we should push back on the spec for it...
Attachment #764582 - Attachment is obsolete: true
Attachment #764582 - Flags: review?(roc)
Attachment #768142 - Flags: review?(roc)
Attachment #764584 - Attachment is obsolete: true
Attachment #764584 - Flags: review?(khuey)
Attachment #768145 - Flags: review?(khuey)
Attachment #764585 - Attachment is obsolete: true
Attachment #768146 - Flags: review+
Comment on attachment 768142 [details] [diff] [review]
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.1

Review of attachment 768142 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/HTMLVideoElement.webidl
@@ +47,5 @@
>  };
> +
> +// https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#idl-def-HTMLVideoElement
> +partial interface HTMLVideoElement {
> +  [Pref="media.mediasource.enabled", Creator]

Remove Creator attribute.
Attachment #768142 - Flags: review?(roc) → review+
Attachment #768142 - Flags: checkin+
Attachment #768143 - Flags: checkin+
Drop [Creator], did this in my local changes but forgot and landed the patches from this bug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/59857a435c3b
Comment on attachment 768145 [details] [diff] [review]
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.1

Review of attachment 768145 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +314,5 @@
> +  NS_ASSERTION(IsMediaSourceURI(aURI), "Only call this with mediasource URIs");
> +
> +  *aSource = nullptr;
> +
> +  nsCOMPtr<mozilla::dom::MediaSource> source = do_QueryInterface(GetDataObject(aURI));

This doesn't actually do what you think it does.  mozilla::dom::MediaSource is not an interface, so do_QueryInterface can't QI to it.  MediaSource singly inherits from nsIDOMEventTarget (via nsDOMEventTargetHelper and friends) so the do_QueryInterface is actually going to QI to nsIDOMEventTarget and then static_cast to a MediaSource.  This works today but if someone ever adds another type for GetDataObject to return that inherits from nsIDOMEventTargetHelper this will do very bad things.

You should use the declare/define iid macros on MediaSource to make it actually an interface.
Attachment #768145 - Flags: review?(khuey) → review-
Attachment #768142 - Flags: checkin+
Attachment #768143 - Flags: checkin+
Fix build error by removing unused GetFrameStatistics from HTMLMediaElement.
Attachment #768142 - Attachment is obsolete: true
Attachment #768770 - Flags: review+
Add IID goop to make MediaSource a real interface.
Attachment #768145 - Attachment is obsolete: true
Attachment #768772 - Flags: review?(khuey)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1df349338b08 for Windows build failures that may well just have been a need to clobber, dunno.
Try push of the same changesets that were just backed out:

https://tbpl.mozilla.org/?tree=Try&rev=403c3ca5ee70
Whiteboard: [leave open]
Also Android mochitest-2 bustage.
That try push is fairly green, so It looks like it just needs a clobber.

The Android M2 is real though, and not caught by the try test, so thanks for pointing that out:  https://tbpl.mozilla.org/php/getParsedLog.php?id=24818421&tree=Mozilla-Inbound
Depends on: 889712
You need to log in before you can comment on or make changes to this bug.