Last Comment Bug 855130 - Implement enough of the Media Source Extensions API to run the demo app
: Implement enough of the Media Source Extensions API to run the demo app
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla25
Assigned To: Matthew Gregan [:kinetik]
:
: Maire Reavy [:mreavy]
Mentors:
http://html5-demos.appspot.com/static...
Depends on: 889712
Blocks: MSE
  Show dependency treegraph
 
Reported: 2013-03-26 16:59 PDT by Matthew Gregan [:kinetik]
Modified: 2013-08-21 14:19 PDT (History)
25 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (applies to aed1da04448d) (56.80 KB, patch)
2013-03-26 18:14 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
wip v2 (applies to aed1da04448d) (56.29 KB, patch)
2013-04-10 19:47 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
fix some build issues found on try (2.69 KB, patch)
2013-04-11 07:38 PDT, Geoff Brown [:gbrown]
no flags Details | Diff | Splinter Review
wip v3 (applies to 7b8ed29c6bc0) (56.61 KB, patch)
2013-04-11 21:26 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
minimal patch for leak debugging v0 (24.16 KB, patch)
2013-05-01 23:03 PDT, Matthew Gregan [:kinetik]
khuey: feedback-
Details | Diff | Splinter Review
traverse protocolhandler patch v0 (3.26 KB, patch)
2013-05-02 17:55 PDT, Matthew Gregan [:kinetik]
khuey: feedback+
kinetik: checkin+
Details | Diff | Splinter Review
wip v4 (applies to 7e3a4ebcf067) (72.12 KB, patch)
2013-06-03 20:47 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
wip v4.1 (applies to 22cb668fd727) (84.10 KB, patch)
2013-06-04 23:46 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v4.2 (applies to 22cb668fd727) (88.96 KB, patch)
2013-06-11 21:49 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v4.2 (applies to cc35f8929768) (88.99 KB, patch)
2013-06-11 22:14 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v4.4 (applies to 1790f40f71f0) (341.42 KB, patch)
2013-06-17 23:06 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
hostobjectprotocolhandler patch v1.0 (applies to 1790f40f71f0) (17.93 KB, patch)
2013-06-17 23:07 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
MSE p1 VideoPlaybackQuality v1.0 (11.98 KB, patch)
2013-06-18 23:33 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
MSE p2 MediaSource v1.0 (49.79 KB, patch)
2013-06-18 23:34 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0 (16.26 KB, patch)
2013-06-18 23:35 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
MSE p4 MediaElement integration v1.0 (7.92 KB, patch)
2013-06-18 23:37 PDT, Matthew Gregan [:kinetik]
roc: review+
Details | Diff | Splinter Review
MSE p5 MSE tests v1.0 (274.49 KB, patch)
2013-06-18 23:37 PDT, Matthew Gregan [:kinetik]
roc: review+
Details | Diff | Splinter Review
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.1 (17.45 KB, patch)
2013-06-26 21:21 PDT, Matthew Gregan [:kinetik]
roc: review+
Details | Diff | Splinter Review
MSE p2 - Implement a minimal working subset of the MediaSource Extensions API. v1.1 (49.69 KB, patch)
2013-06-26 21:22 PDT, Matthew Gregan [:kinetik]
roc: review+
Details | Diff | Splinter Review
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.1 (16.51 KB, patch)
2013-06-26 21:24 PDT, Matthew Gregan [:kinetik]
khuey: review-
Details | Diff | Splinter Review
MSE p4 Integrate MediaSource into HTMLMediaElement. v1.1 (8.17 KB, patch)
2013-06-26 21:25 PDT, Matthew Gregan [:kinetik]
kinetik: review+
Details | Diff | Splinter Review
MSE p5 Add initial tests for MediaSource Extensions implementation. v1.1 (274.87 KB, patch)
2013-06-26 21:25 PDT, Matthew Gregan [:kinetik]
kinetik: review+
Details | Diff | Splinter Review
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.2 (16.71 KB, patch)
2013-06-27 22:55 PDT, Matthew Gregan [:kinetik]
kinetik: review+
Details | Diff | Splinter Review
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.2 (19.17 KB, patch)
2013-06-27 22:57 PDT, Matthew Gregan [:kinetik]
khuey: review+
Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2013-03-26 16:59:49 PDT
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.
Comment 1 Matthew Gregan [:kinetik] 2013-03-26 18:14:45 PDT
Created attachment 729909 [details] [diff] [review]
wip (applies to aed1da04448d)

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.
Comment 2 Yury Delendik (:yury) 2013-04-08 18:31:01 PDT
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()
+{
Comment 3 Matthew Gregan [:kinetik] 2013-04-10 19:47:38 PDT
Created attachment 736095 [details] [diff] [review]
wip v2 (applies to aed1da04448d)

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.
Comment 4 Geoff Brown [:gbrown] 2013-04-11 07:38:50 PDT
Created attachment 736286 [details] [diff] [review]
fix some build issues found on try

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.)
Comment 5 Geoff Brown [:gbrown] 2013-04-11 10:54:43 PDT
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.
Comment 6 Matthew Gregan [:kinetik] 2013-04-11 21:26:28 PDT
Created attachment 736650 [details] [diff] [review]
wip v3 (applies to 7b8ed29c6bc0)

Rebased, integrated gbrown's warning fixes, possibly fixed the stalling bug (bogus blocking calculation in Read()).
Comment 7 Geoff Brown [:gbrown] 2013-04-12 10:32:54 PDT
The stalling bug I reported in Comment 5 seems to be resolved by wip-v3 -- it works great now!
Comment 9 Yury Delendik (:yury) 2013-04-24 12:46:23 PDT
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 .
Comment 10 Yury Delendik (:yury) 2013-04-29 12:49:23 PDT
(FF/Mac/gstreamer) fragmented MP4 does not start playback until endOfStream is called, see example at http://async5.org/moz/story.html
Comment 11 Matthew Gregan [:kinetik] 2013-05-01 23:03:59 PDT
Created attachment 744478 [details] [diff] [review]
minimal patch for leak debugging v0

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?
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2013-05-01 23:28:04 PDT
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?
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-05-02 10:37:24 PDT
(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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2013-05-02 10:51:51 PDT
Matthew, are you OK giving that a shot?
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-05-02 10:57:19 PDT
I can find someone to do that if necessary.
Comment 16 Matthew Gregan [:kinetik] 2013-05-02 17:55:15 PDT
Created attachment 744941 [details] [diff] [review]
traverse protocolhandler patch v0

Thanks for the explanations.  I'm not really sure what I'm doing here, but this seems to work.
Comment 17 Matthew Gregan [:kinetik] 2013-05-21 20:22:33 PDT
14:36 < khuey|tw> kinetik: fwiw you can treat that f+ as an r+ if you like
Comment 18 Matthew Gregan [:kinetik] 2013-05-21 23:46:45 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc3ef777ded
Comment 20 Matthew Gregan [:kinetik] 2013-05-22 00:56:36 PDT
Simple fix: Traverse needs to null-check gDataTable.
Comment 21 Matthew Gregan [:kinetik] 2013-05-22 21:17:22 PDT
Relanded with null-check (looks okay on try: https://tbpl.mozilla.org/?tree=Try&rev=c0224e2ec0b0):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8920210273
Comment 22 Ed Morley [:emorley] 2013-05-23 04:52:46 PDT
https://hg.mozilla.org/mozilla-central/rev/9c8920210273
Comment 23 Matthew Gregan [:kinetik] 2013-06-03 20:47:23 PDT
Created attachment 757783 [details] [diff] [review]
wip v4 (applies to 7e3a4ebcf067)

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.
Comment 24 Matthew Gregan [:kinetik] 2013-06-03 22:07:21 PDT
(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.
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2013-06-04 10:11:52 PDT
Does this have a spec?

I'd also appreciate a diff with 8 lines of context, if possible.
Comment 26 Matthew Gregan [:kinetik] 2013-06-04 23:46:19 PDT
Created attachment 758422 [details] [diff] [review]
wip v4.1 (applies to 22cb668fd727)

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.
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2013-06-08 04:34:08 PDT
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?
Comment 28 Matthew Gregan [:kinetik] 2013-06-10 22:06:32 PDT
(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.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-10 22:09:04 PDT
(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.
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2013-06-11 09:34:26 PDT
(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
Comment 31 Matthew Gregan [:kinetik] 2013-06-11 21:49:30 PDT
Created attachment 761288 [details] [diff] [review]
patch v4.2 (applies to 22cb668fd727)

Thanks for the feedback!  This patch addresses everything that I didn't cover in comment 28.
Comment 32 Matthew Gregan [:kinetik] 2013-06-11 22:14:45 PDT
Created attachment 761295 [details] [diff] [review]
patch v4.2 (applies to cc35f8929768)

Same, but rebased to current tip (minor changes to content/media/moz.build).
Comment 33 :Ms2ger (⌚ UTC+1/+2) 2013-06-15 00:27:23 PDT
(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+.)
Comment 34 Matthew Gregan [:kinetik] 2013-06-16 23:28:10 PDT
Should I convert the request to a feedback? and maybe add Boris for review?
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-17 00:47:21 PDT
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.
Comment 36 Matthew Gregan [:kinetik] 2013-06-17 23:06:09 PDT
Created attachment 763990 [details] [diff] [review]
patch v4.4 (applies to 1790f40f71f0)

Split HostObjectProtocolHandler and URL changes out.  Improved mochitest to cover all of the initial functionality this bug targets.
Comment 37 Matthew Gregan [:kinetik] 2013-06-17 23:07:35 PDT
Created attachment 763991 [details] [diff] [review]
hostobjectprotocolhandler patch v1.0 (applies to 1790f40f71f0)

Applies on top of main patch.  Add MediaSource to HostObjectProtocolHandler and extend URL::createObjectURL to support MediaSource.
Comment 38 Matthew Gregan [:kinetik] 2013-06-18 22:44:17 PDT
Comment on attachment 763990 [details] [diff] [review]
patch v4.4 (applies to 1790f40f71f0)

Obsoleting these patches and splitting into smaller chunks.
Comment 39 Matthew Gregan [:kinetik] 2013-06-18 23:33:43 PDT
Created attachment 764582 [details] [diff] [review]
MSE p1 VideoPlaybackQuality v1.0
Comment 40 Matthew Gregan [:kinetik] 2013-06-18 23:34:22 PDT
Created attachment 764583 [details] [diff] [review]
MSE p2 MediaSource v1.0
Comment 41 Matthew Gregan [:kinetik] 2013-06-18 23:35:40 PDT
Created attachment 764584 [details] [diff] [review]
MSE p3 HostObjectProtocolHandler/createObjectURL integration v1.0
Comment 42 Matthew Gregan [:kinetik] 2013-06-18 23:37:09 PDT
Created attachment 764585 [details] [diff] [review]
MSE p4 MediaElement integration v1.0
Comment 43 Matthew Gregan [:kinetik] 2013-06-18 23:37:49 PDT
Created attachment 764586 [details] [diff] [review]
MSE p5 MSE tests v1.0
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-19 00:18:50 PDT
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 45 :Ms2ger (⌚ UTC+1/+2) 2013-06-19 01:19:28 PDT
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 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-19 03:17:27 PDT
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 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-19 03:19:22 PDT
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.
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-19 03:23:49 PDT
Comment on attachment 764586 [details] [diff] [review]
MSE p5 MSE tests v1.0

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

Very nice!
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2013-06-19 05:57:51 PDT
> 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.
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-19 06:03:44 PDT
I think this should probably be a live object.
Comment 51 Matthew Gregan [:kinetik] 2013-06-23 18:52:57 PDT
(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.
Comment 52 Matthew Gregan [:kinetik] 2013-06-23 19:28:19 PDT
(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 53 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-06-24 10:05:16 PDT
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?
Comment 54 :Ms2ger (⌚ UTC+1/+2) 2013-06-24 10:39:10 PDT
(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.
Comment 55 Matthew Gregan [:kinetik] 2013-06-24 19:40:14 PDT
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?
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-25 21:47:31 PDT
(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]...
Comment 57 Boris Zbarsky [:bz] (still a bit busy) 2013-06-25 22:34:36 PDT
Huh.  I thought we disallowed that!  We should.
Comment 58 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-26 19:03:10 PDT
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.
Comment 59 Boris Zbarsky [:bz] (still a bit busy) 2013-06-26 20:26:25 PDT
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.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-26 20:38:08 PDT
Oh, we can remove [Creator] but still return a new object every time? Great, let's do that here then :-).
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2013-06-26 20:46:42 PDT
[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...
Comment 62 Matthew Gregan [:kinetik] 2013-06-26 21:21:44 PDT
Created attachment 768142 [details] [diff] [review]
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.1
Comment 63 Matthew Gregan [:kinetik] 2013-06-26 21:22:50 PDT
Created attachment 768143 [details] [diff] [review]
MSE p2 - Implement a minimal working subset of the MediaSource Extensions API. v1.1
Comment 64 Matthew Gregan [:kinetik] 2013-06-26 21:24:22 PDT
Created attachment 768145 [details] [diff] [review]
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.1
Comment 65 Matthew Gregan [:kinetik] 2013-06-26 21:25:05 PDT
Created attachment 768146 [details] [diff] [review]
MSE p4 Integrate MediaSource into HTMLMediaElement. v1.1
Comment 66 Matthew Gregan [:kinetik] 2013-06-26 21:25:41 PDT
Created attachment 768147 [details] [diff] [review]
MSE p5 Add initial tests for MediaSource Extensions implementation. v1.1
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-06-26 22:30:50 PDT
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.
Comment 69 Matthew Gregan [:kinetik] 2013-06-27 21:37:44 PDT
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 70 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-06-27 21:48:22 PDT
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.
Comment 72 Matthew Gregan [:kinetik] 2013-06-27 22:55:29 PDT
Created attachment 768770 [details] [diff] [review]
MSE p1 - Implement HTMLVideoElement's VideoPlaybackQuality (from MediaSource Extensions). v1.2

Fix build error by removing unused GetFrameStatistics from HTMLMediaElement.
Comment 73 Matthew Gregan [:kinetik] 2013-06-27 22:57:25 PDT
Created attachment 768772 [details] [diff] [review]
MSE p3 Implement URL::createObjectURL overload for MediaSources. v1.2

Add IID goop to make MediaSource a real interface.
Comment 74 Matthew Gregan [:kinetik] 2013-06-27 23:01:35 PDT
https://tbpl.mozilla.org/?tree=Try&rev=682bfc7c7c3c
Comment 76 Phil Ringnalda (:philor) 2013-07-01 21:25:43 PDT
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.
Comment 77 Matthew Gregan [:kinetik] 2013-07-01 21:31:26 PDT
Try push of the same changesets that were just backed out:

https://tbpl.mozilla.org/?tree=Try&rev=403c3ca5ee70
Comment 78 Phil Ringnalda (:philor) 2013-07-01 23:18:34 PDT
Also Android mochitest-2 bustage.
Comment 79 Matthew Gregan [:kinetik] 2013-07-02 03:35:12 PDT
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

Note You need to log in before you can comment on or make changes to this bug.