Ralph Giles (:rillian) Reporter
	Description • 12 years ago

Media files can have multiple audio and video tracks. Right now we just play the first track of each type we find. We should support switching between the available tracks, as described in http://dev.w3.org/html5/spec/media-elements.html#media-resources-with-multiple-media-tracks and in our native controls ui.

Alternate tracks can provide audio in other languages, audio descriptions for better accessibility, alternate visual material like slides accompanying a presentation and so one.

	Ralph Giles (:rillian) Reporter
	Comment 1 • 12 years ago

Note I'm not especially a fan of the MediaControllerGroup part of the spec. Implementing the multitrack aspects for audio and video streams is a smaller piece of work.

	Brendan Long
	Comment 3 • 11 years ago

I may be able to do this. I'm wondering how we're supposed to implement this IDL for the TrackEvent though:

interface TrackEvent : Event {
  readonly attribute (VideoTrack or AudioTrack or TextTrack) track;
};

dictionary TrackEventInit : EventInit {
  (VideoTrack or AudioTrack or TextTrack) track;
};

http://www.whatwg.org/specs/web-apps/current-work/#trackevent

Also, AudioTrack and VideoTrack aren't event targets, so what's the correct type to derive from?

	Andrew Quartey [:drexler]
	Comment 4 • 11 years ago

(In reply to Brendan Long from comment #3)
> I may be able to do this. I'm wondering how we're supposed to implement this
> IDL for the TrackEvent though:
> 
> interface TrackEvent : Event {
>   readonly attribute (VideoTrack or AudioTrack or TextTrack) track;
> };
> 
> dictionary TrackEventInit : EventInit {
>   (VideoTrack or AudioTrack or TextTrack) track;
> };
> 
> http://www.whatwg.org/specs/web-apps/current-work/#trackevent

IIRC, the current implementation of webidl supports unions. 

> Also, AudioTrack and VideoTrack aren't event targets, so what's the correct
> type to derive from?

It's not specified, so no need to worry about that: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#audiotracklist.

	Shelly Lin [:shelly] Assignee
	Comment 5 • 10 years ago

Hi Brendan, 
Is there any update on the implementation?

	Brendan Long
	Comment 6 • 10 years ago

Sorry, I got distracted by some other things and completely forgot about this. I can look at it again, but it will probably be two weeks or so. If someone else wants to take this on I won't mind. If not, I've added it back to my todo list and should get to it once things settle down in about two weeks.

	Brendan Long
	Comment 7 • 10 years ago

I had some time during my lunch break and decided to take a look at this again, to at least make it simpler for anyone else to implement. I was planning to simplify this by creating Track and TrackList base classes (id, label, language is identical in *Track, basically everything is the same for *TrackList).

Unfortunately I'm getting an error with the union in TrackEvent, and I can't find any other code that uses it, so I wonder if it's just broken:

 0:51.49 In file included from /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings30.cpp:50:0:
 0:51.49 /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/TrackEvent.cpp: In static member function ‘static already_AddRefed<mozilla::dom::TrackEvent> mozilla::dom::TrackEvent::Constructor(mozilla::dom::EventTarget*, const nsAString_internal&, const mozilla::dom::TrackEventInit&)’:
 0:51.49 /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/TrackEvent.cpp:61:13: error: no match for ‘operator=’ (operand types are ‘mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack’ and ‘const mozilla::dom::Optional<mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack>’)
 0:51.49    e->mTrack = aEventInitDict.mTrack;
 0:51.49              ^
 0:51.49 /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/TrackEvent.cpp:61:13: note: candidate is:
 0:51.49 In file included from ../../dist/include/mozilla/dom/TrackEventBinding.h:12:0,
 0:51.49                  from ../../dist/include/mozilla/dom/TrackEvent.h:15,
 0:51.49                  from /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/TrackEvent.cpp:10,
 0:51.49                  from /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings30.cpp:50:
 0:51.49 ../../dist/include/mozilla/dom/UnionTypes.h:4946:8: note: void mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack::operator=(mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack) <deleted>
 0:51.49    void operator=(const OwningVideoTrackOrAudioTrackOrTextTrack) MOZ_DELETE;
 0:51.49         ^
 0:51.49 ../../dist/include/mozilla/dom/UnionTypes.h:4946:8: note:   no known conversion for argument 1 from ‘const mozilla::dom::Optional<mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack>’ to ‘mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack’
 0:51.49 ../../dist/include/mozilla/dom/UnionTypes.h: In member function ‘void mozilla::dom::TrackEvent::GetTrack(mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack&) const’:
 0:51.49 ../../dist/include/mozilla/dom/UnionTypes.h:4946:8: error: ‘void mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack::operator=(mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack)’ is private
 0:51.49 In file included from /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/UnifiedBindings30.cpp:50:0:
 0:51.49 /home/blong/workspace/gecko-dev/obj-x86_64-unknown-linux-gnu/dom/bindings/TrackEvent.cpp:76:11: error: within this context
 0:51.49    aRetVal = mTrack;

The issue seems to be these two (generated) functions:

already_AddRefed<TrackEvent>
TrackEvent::Constructor(mozilla::dom::EventTarget* aOwner, const nsAString& aType, const TrackEventInit& aEventInitDict)
{
  nsRefPtr<TrackEvent> e = new TrackEvent(aOwner);
  bool trusted = e->Init(aOwner);
  e->InitEvent(aType, aEventInitDict.mBubbles, aEventInitDict.mCancelable);
  e->mTrack = aEventInitDict.mTrack.Value();
  e->SetTrusted(trusted);
  return e.forget();
}

void
TrackEvent::GetTrack(OwningVideoTrackOrAudioTrackOrTextTrack& aRetVal) const
{
  aRetVal = mTrack;
}

OwningVideoTrackOrAudioTrackOrTextTrack (also generated) has no operator=, and Optional<OwningVideoTrackOrAudioTrackOrTextTrack> can't be implicitly converted to OwningVideoTrackOrAudioTrackOrTextTrack.

	Andrew Quartey [:drexler]
	Comment 8 • 10 years ago

> Unfortunately I'm getting an error with the union in TrackEvent, and I can't
> find any other code that uses it, so I wonder if it's just broken:

TrackEvent as implemented only caters to the TextTrack and not the full spec per se. See: http://dxr.mozilla.org/mozilla-central/source/dom/webidl/TrackEvent.webidl

	Brendan Long
	Comment 9 • 10 years ago

(In reply to Andrew Quartey [:drexler] from comment #8)
> > Unfortunately I'm getting an error with the union in TrackEvent, and I can't
> > find any other code that uses it, so I wonder if it's just broken:
> 
> TrackEvent as implemented only caters to the TextTrack and not the full spec
> per se. See:
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/TrackEvent.webidl

I updated TrackEvent.webidl in my patch to match the new definition:

interface TrackEvent : Event {
  readonly attribute (VideoTrack or AudioTrack or TextTrack) track;
};

dictionary TrackEventInit : EventInit {
  (VideoTrack or AudioTrack or TextTrack) track;
};

	Brendan Long
	Comment 10 • 10 years ago

I looked at another class that uses unions from WebIDL (HTMLOptionsCollection), and it has HTMLOptionsCollection.{h,cpp}, instead of using the generated one, so maybe I just need to write real TrackEvent.{h,cpp} files instead of using generated ones. Or fix the generator..

	Brendan Long
	Comment 11 • 10 years ago

I have no idea why some WebIDL files cause files to be generated and some don't, but I looked through some other events and they all set default values, even with the standard doesn't, so I changed the file to this and now it works:

[Constructor(DOMString type, optional TrackEventInit eventInitDict)]
interface TrackEvent : Event {
  readonly attribute object track;
};

dictionary TrackEventInit : EventInit {
  object? track = null;
};

I also don't know why (VideoTrack or TextTrack or AudioTrack) causes the copy constructor to be MOZ_DELETE'd, but with the above WebIDL, I can make a basic API patch compile.

	Brendan Long
	Comment 12 • 10 years ago

Here's the compiling patch. I'll try to simplify it and get things hooked up, but I only have one day until I leave for about two weeks. Hopefully this will be helpful if anyone else wants to implement this during that time.

	Brendan Long
	Comment 13 • 10 years ago

I've been looking through the code, and it looks like the next bits are going to be in MediaDecoder and the related classes. Probably:

 1. Add a callback on playbin's audio-changed, video-changed, and text-changed.
 2. Add some Notify*TrackAdded() and Notify*TrackRemoved() callbacks to MediaDecoderOwner.
 3. Make tracks able to somehow to tell the MediaDecoder to change the rendered track.
 4. Do similar work to step 1 for decoders besides GStreamer.

	Brendan Long
	Comment 14 • 10 years ago

Rather than post a bunch of tiny patches whenever I make progress, you can see my current progress on this branch:

https://github.com/brendanlong/gecko-dev/commits/audio-and-video-tracks

	Shelly Lin [:shelly] Assignee
	Comment 15 • 10 years ago

Thank you Brendan!

It is great to have a heads-up of what has been done so far, so that no resource will duplicated.

	Shelly Lin [:shelly] Assignee
	Comment 16 • 10 years ago

Hi Brendan, this feature is marked as a must have for the TV project, therefore, we would like to land it no later than end of June. If your hands are full, would you mind if I have a go at this bug :)?

	Brendan Long
	Comment 17 • 10 years ago

(In reply to Shelly Lin [:shelly] from comment #16)
> Hi Brendan, this feature is marked as a must have for the TV project,
> therefore, we would like to land it no later than end of June. If your hands
> are full, would you mind if I have a go at this bug :)?

Yeah, go for it. I've been a lot busier than I thought I would be. There are two patches in this GitHub repo you might find useful to get started:

https://github.com/brendanlong/gecko-dev/commits/audio-and-video-tracks

	Shelly Lin [:shelly] Assignee
	Comment 18 • 10 years ago

The implementation is base on:
http://www.w3.org/TR/html5/embedded-content-0.html#audiotracklist-and-videotracklist-objects

I also notice that there is a recent change to the TextTrack and its related modules (Bug 950308), I'm puzzled by it dropping the SetIsDOMBinding() and GetParentObject(). 

Also, AudioTrack and VideoTrack share lots of similar attributes, so I created a MediaTrack to be their base element, as well as the AudioTrackList, VideoTrackList, and its event dispatching. If possible, I'd like them to be shared with TextTrack and TextTrackList too.

Could you give me some feedback to see if I'm heading the right direction?

	Shelly Lin [:shelly] Assignee
	Comment 19 • 10 years ago

Base on:
http://www.w3.org/TR/html5/embedded-content-0.html#trackevent

The auto-gen of event doesn't seem to support union types.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 10 years ago

Comment on attachment 8407276 [details] [diff] [review]
Part1: Implement audio/video track API

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

This looks very good.

::: content/media/MediaTrack.cpp
@@ +128,5 @@
> +
> +  event->SetTrusted(true);
> +
> +  nsCOMPtr<nsIRunnable> eventRunner = new TrackEventRunner(this, event);
> +  NS_DispatchToMainThread(eventRunner, NS_DISPATCH_NORMAL);

Remove NS_DISPATCH_NORMAL parameter

::: content/media/MediaTrack.h
@@ +70,5 @@
> +  nsString mLabel;
> +  nsString mLanguage;
> +};
> +
> +class MediaTrackList : public DOMEventTargetHelper

Put MediaTrackList in its own file

::: content/media/VideoTrack.cpp
@@ +13,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED_2(VideoTrack,
> +                                     DOMEventTargetHelper,
> +                                     mParent,
> +                                     mList)

mParent and mList should be traced by MediaTrack, not its subclasses.

@@ +58,5 @@
> +    // Un-select the other video tracks.
> +    for (uint32_t i = 0; i < list.Length(); ++i) {
> +      VideoTrack* track = list[i];
> +      if (track->Selected()) {
> +        track->SetSelected(false);

Just do SetSelected unconditionally. Also, you're deselecting all video tracks ... one of them should be marked selected.

Actually, I think it would be better to remove mSelected from VideoTrack. It's redundant. Make VideoTrack::GetSelected() check which track is selected in its parent list.

::: content/media/VideoTrackList.cpp
@@ +11,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED_2(VideoTrackList,
> +                                     DOMEventTargetHelper,
> +                                     mGlobal,
> +                                     mTracks)

mGlobal and mTracks should be traced by MediaTrackList, not in its subclasses.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 10 years ago

Comment on attachment 8407279 [details] [diff] [review]
Part2: Create TrackEvent

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

I think you need to remove TrackEvent.h from GENERATED_EVENTS_WEBIDL_FILES in some moz.build.

Otherwise looks good.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 10 years ago

You will need a DOM peer to review these patches.

	Shelly Lin [:shelly] Assignee
	Comment 23 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Comment on attachment 8407279 [details] [diff] [review]
> Part2: Create TrackEvent
> 
> Review of attachment 8407279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you need to remove TrackEvent.h from GENERATED_EVENTS_WEBIDL_FILES
> in some moz.build.
> 
> Otherwise looks good.

I must have missed it when resolving the merging conflicts.

Thanks for all the feedback Roc :)

	Shelly Lin [:shelly] Assignee
	Comment 24 • 10 years ago

Comment on attachment 8407276 [details] [diff] [review]
Part1: Implement audio/video track API

Hi Boris,

comment 18 has briefly listed the initiative of this implementation, could you give me some feedback regards to the DOM implementation?

	Shelly Lin [:shelly] Assignee
	Comment 25 • 10 years ago

Comment on attachment 8407279 [details] [diff] [review]
Part2: Create TrackEvent

The current TrackEvent is using auto-gen event, but seems like the auto-gen doesn't support union types. Could you give me some feedback to see if this TrackEvent is heading to the right direction? Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 10 years ago

Comment on attachment 8407276 [details] [diff] [review]
Part1: Implement audio/video track API

Most of this looks great.  I have just a few comments:

I don't see where we ever call the AudioTrackList/MediaTrackList constructors.  Why do those take nsISupports instead of, say nsPIDOMWindow?  If they did the latter, then they could pass it to the corresponding constructor of DOMEventTargetHelper, which automatically does SetIsDOMBinding().

Why does MediaTrack inherit from DOMEventTargetHelper?  Neither AudioTrack nor VideoTrack are EventTargets, right?

It would probably be better if MediaTrack handled cycle collection of its members instead of forcing subclasses to do it.  Similar for MediaTrackList. 

Do we really want to warn in AudioTrack/VideoTrack::SetEnabled if the same value is being set or if we have no mList?

AudioTrack::Enabled should probably be a const method.

This patch seems to be missing changes to TrackEventInit that CreateAndDispatchTrackEventRunner depends on.

mBubbles and mCancelable should already be false, so no need to set them false explicitly.

You don't need to declare the pure-virtual WrapObject on MediaTrack and MediaTrackList, as far as I can tell.  It's already pure virtual on the superclasses.

For the WebIDL, do we want to reference the whatwg version, or are they identical enough that it doesn't matter?

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 10 years ago

Comment on attachment 8407279 [details] [diff] [review]
Part2: Create TrackEvent

> but seems like the auto-gen doesn't support union types.

We sure try to; see bug 949264.  What didn't work when you tried it?

I can take a look at this if it becomes necessary, but let's try to make the codegen work for us first...

> dictionary TrackEventInit : EventInit
>+  (VideoTrack or AudioTrack or TextTrack) track;

This shouldn't lose the nullability or the = null, right?  Certainly the C++ code assumes this always has a value.

Yes, I know the IDL in the spec doesn't have that bit.  The IDL in the spec is wrong and should be fixed; please file a spec issue.

Similarly, the IDL for TrackEvent should have "track" as a nullable attribute, since the spec prose clearly says it can be null...

	Shelly Lin [:shelly] Assignee
	Comment 28 • 10 years ago

In reply to the first question, the constructor of audio track list and video track list is in this wip patch.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 10 years ago

Comment on attachment 8408042 [details] [diff] [review]
Part 3: Add track list to HTMLMediaElement [WIP]

Ah, yes.  So you could QI OwnerDoc()->GetParentObject() to nsPIDOMWindow and use that.

Are the aTestLabel/aTestLanguage bits final?  I'd assume no?  ;)

The rest looks fine here.

	Shelly Lin [:shelly] Assignee
	Comment 30 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #26)
> Comment on attachment 8407276 [details] [diff] [review]
> Part1: Implement audio/video track API
> 
> Most of this looks great.  I have just a few comments:
> 
> I don't see where we ever call the AudioTrackList/MediaTrackList
> constructors.  Why do those take nsISupports instead of, say nsPIDOMWindow? 
> If they did the latter, then they could pass it to the corresponding
> constructor of DOMEventTargetHelper, which automatically does
> SetIsDOMBinding().
> 
Sure :) 
I also notice the change of using nsPIDOMWindow in Bug 950308, I just don't know their difference.

> Why does MediaTrack inherit from DOMEventTargetHelper?  Neither AudioTrack
> nor VideoTrack are EventTargets, right?
> 
I plan to have the TextTrack inherit from MediaTrack too, since the text, audio and video track share quite a lot common attributes and events, and the TextTrack is a EventTarget.

	Shelly Lin [:shelly] Assignee
	Comment 31 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8408042 [details] [diff] [review]
> Part 3: Add track list to HTMLMediaElement [WIP]
> 
> Ah, yes.  So you could QI OwnerDoc()->GetParentObject() to nsPIDOMWindow and
> use that.
> 
> Are the aTestLabel/aTestLanguage bits final?  I'd assume no?  ;)
> 
Ha, of course not! That's why is not there in the first place.

> The rest looks fine here.

Thanks for the feedback overall :)

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 10 years ago

> I just don't know their difference.

DOMEventTargetHelper has a constructor that takes an nsPIDOMWindow.  This constructor (1) knows to call SetIsDOMBinding() and (2) makes sure the event target is associated with that window for some internal measurements.

> I plan to have the TextTrack inherit from MediaTrack too

Hmm.  You could have it inherit from both DOMEventTargetHelper and MediaTrack.... but that might get a bit confusing with the cycle collection.  OK, let's leave MediaTrack inheriting from DOMEventTargetHelper, with some comments explaining what's going on.

	Shelly Lin [:shelly] Assignee
	Comment 33 • 10 years ago

Hi :bz, I've addressed most of your previous feedback, thanks a lot :)

Actually, after I've made the TrackEvent be a code-gen event, the compiler is complaining about the operator= of mozilla::dom::OwningVideoTrackOrAudioTrackOrTextTrack is private. How can I make it a public method?

	Shelly Lin [:shelly] Assignee
	Comment 34 • 10 years ago

Please find my comment inline.

	Shelly Lin [:shelly] Assignee
	Comment 35 • 10 years ago

Comment on attachment 8410179 [details] [diff] [review]
Part 3: Add track list to HTMLMediaElement [WIP]

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2024,5 @@
>  
>    mPaused.SetOuter(this);
>  
> +  // TODO: Use lazy loading?
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(OwnerDoc()->GetParentObject());

I notice that you've suggested constructing TextTrackList with OwnerDoc()->GetScriptHandlingObject(), should I do the same thing here?

	Shelly Lin [:shelly] Assignee
	Comment 36 • 10 years ago

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #27)
> Comment on attachment 8407279 [details] [diff] [review]
> Part2: Create TrackEvent
> 
> > but seems like the auto-gen doesn't support union types.
> 
> We sure try to; see bug 949264.  What didn't work when you tried it?
> 
> I can take a look at this if it becomes necessary, but let's try to make the
> codegen work for us first...
> 

I think I've traced down the problem here, if you have a union type attribute, it is not copy constructible (I got this conclusion by checking all the union type attributes from our webidls). 

There're several union type assignments in TrackEvent.cpp (generated by auto-gen), result in causing compile errors.

Hi David, could you provide some ideas here? (I searched the bugs and saw you did a lot union type related bugs)

Thanks:)

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 10 years ago

Unions are _sometimes_ copy constructible.  In particular, see bug 925737.

I'm not sure why we made interface types not copy-constructible there; probably just no use cases at the time and caution.  We should probably just fix that.

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 10 years ago

> OwnerDoc()->GetScriptHandlingObject(), should I do the same thing here?

GetParentObject() makes sense if this pointer will be used as the parent object.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 10 years ago

Shelly, does that address your feedback requests?  If not, what sort of feedback are you looking for?

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 10 years ago

I filed bug 1000944 to make the union bit work.

	Shelly Lin [:shelly] Assignee
	Comment 41 • 10 years ago

(In reply to Boris Zbarsky [:bz] (on PTO, reviews are slow) from comment #39)
> Shelly, does that address your feedback requests?  
Yes.

(In reply to comment 37 together)
I found some unions copy-constructible, but it happened to be not in this case. Thanks for fixing the auto-gen :)

	Shelly Lin [:shelly] Assignee
	Comment 44 • 10 years ago

A more refined version.

	Shelly Lin [:shelly] Assignee
	Comment 45 • 10 years ago

Enable the attributes audioTracks and videoTracks of media elements, but only valid when the media resources are fetched from media stream.

	Shelly Lin [:shelly] Assignee
	Comment 46 • 10 years ago

TODO: Firing "removetrack" events.

According to the w3c spec:

If at any time the user agent learns that an audio or video track has ended and all media data relating to that track corresponds to parts of the media timeline that are before the earliest possible position, the user agent may queue a task to first remove the track from the audioTracks attribute's AudioTrackList object or the videoTracks attribute's VideoTrackList object as appropriate and then fire a trusted event with the name removetrack, that does not bubble and is not cancelable, and that uses the TrackEvent interface, with the track attribute initialized to the AudioTrack or VideoTrack object representing the track, at the media element's aforementioned AudioTrackList or VideoTrackList object.

	Shelly Lin [:shelly] Assignee
	Comment 47 • 10 years ago

Add MediaTrackListListener for media resource to notify when a track has ended or enabled.

	Shelly Lin [:shelly] Assignee
	Comment 48 • 10 years ago

Add the ability to notify media resource whether the track has enabled or not (and the other way around).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 49 • 10 years ago

Comment on attachment 8416499 [details] [diff] [review]
Part1: Implement audio/video track API and update TrackEvent

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

::: content/media/MediaTrackList.cpp
@@ +32,5 @@
> +  nsRefPtr<nsIDOMEvent> mEvent;
> +};
> +
> +MediaTrackListListener::MediaTrackListListener(MediaTrackList* aList)
> +: mMediaTrackList(aList)

indent this line

::: content/media/MediaTrackList.h
@@ +20,5 @@
> + * This is for the media resource to notify its audio track and video track,
> + * when a media-resource-specific track has ended, or whether it has enabled or
> + * not. All notification methods are called from the main thread.
> + */
> +class MediaTrackListListener

Introduce this class in the other patch, where you add the AddListener method.

@@ +37,5 @@
> +  MediaTrackList* mMediaTrackList;
> +};
> +
> +/**
> + * Base class of AudioTrakList and VideoTrackList. The AudioTrackList and

AudioTrackList

@@ +49,5 @@
> +{
> +public:
> +
> +
> +  MediaTrackList(nsPIDOMWindow* aOwnerWindow, HTMLMediaElement* aMediaElement);

Remove blank lines below "public:"

::: content/media/VideoTrack.cpp
@@ +41,5 @@
> +
> +void
> +VideoTrack::SetEnabled(bool aSelected, int aFlags)
> +{
> +  if (aSelected == mSelected) {

Call these aEnabled and mEnabled.

::: content/media/VideoTrack.h
@@ +42,5 @@
> +
> +  // A video track is set selected after fetching the media resource, and no
> +  // events are fired in this case, but the default cases (called from user
> +  // agent) should.
> +  virtual void SetEnabled(bool aEnabled, int aFlags);

It's not clear enough how this interacts with the invariant that only one VideoTrack is enabled at a time. It seems like currently calling SetEnabled(true) ends up calling SetEnabled(false) for all other tracks in the same list. I think we should move responsibility for that out to whoever's calling SetEnabled(true), and remove the SetSelected method.

::: dom/webidl/AudioTrack.webidl
@@ +5,5 @@
> + *
> + * The origin of this IDL file is
> + * http://www.whatwg.org/specs/web-apps/current-work/#audiotrack
> + */
> + 

remove trailing whitespace

::: dom/webidl/VideoTrack.webidl
@@ +5,5 @@
> + *
> + * The origin of this IDL file is
> + * http://www.whatwg.org/specs/web-apps/current-work/#videotrack
> + */
> + 

remove trailing whitespace

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 50 • 10 years ago

Comment on attachment 8416508 [details] [diff] [review]
Part2: Enable track list attributes of MediaElement

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

This patch only enabled AudioTrack/VideoTrack interfaces for media elements that are consuming a MediaStream, right?

Do we really need this for TVs? Why can't TVs just use the MediaStreamTrack interfaces?

::: content/html/content/src/HTMLMediaElement.cpp
@@ +618,5 @@
>    bool fireTimeUpdate = false;
>  
> +  // When aborting the existing loads, empty the objects in audio track list and
> +  // video track list, no events (in particular, no removetrack events) are
> +  // fired as part of this. Ending MediaStreaem sends track end notifications,

MediaStream

@@ +3982,5 @@
> +    } else if (VideoTrack* t = mVideoTrackList->GetTrackById(aId)) {
> +      trackID = t->GetTrackID();
> +    }
> +
> +    mSrcStream->NotifyTrackEnabled(trackID, aEnabled);

I don't think we should be altering the enabled-ness of MediaStreamTracks just because some media element consuming that MediaStreamTrack enabled/disabled its tracks. I think those settings should be independent. After all, the same stream could be consumed by multiple media elements, and we might want to enable/disable the tracks of those elements independently.

::: content/media/MediaStreamTrack.h
@@ +53,5 @@
>  
>    // Notifications from the MediaStreamGraph
> +  void NotifyEnded();
> +
> +  void AddMediaTrackListListener(MediaTrackListListener* aListener);

Document the lifetime of aListener. Who owns it and how do we make sure it gets cleaned up?

	Marco Chen [:mchen]
	Comment 51 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> Do we really need this for TVs? Why can't TVs just use the MediaStreamTrack
> interfaces?

The reasons I know are

  1. As a web developers, I want to perform media control on video tag centrally no matter what is the input resource. 
     (in hybrid TV, the input sources can be digital broadcasting, streaming from IP network and local media files).

  2. There is a draft from IETF [1] which defined "dvb" URI scheme. 
     Then dvb URI can be put into video tag and developer needs to control it in video tag directly.
       

[1] https://tools.ietf.org/html/draft-mcroberts-uri-dvb-04

	Shelly Lin [:shelly] Assignee
	Comment 52 • 10 years ago

Address feedback and update some comments.

	Shelly Lin [:shelly] Assignee
	Comment 53 • 10 years ago

- Move the MediaTrackListListener to this patch.
- I still keep the ability to notify the source media stream when a track has enabled or not, but let it be virtual and do nothing in DOMMediaStream, since it might mostly overridden by some kind of HWOverlayMediaStream.

	Shelly Lin [:shelly] Assignee
	Comment 54 • 10 years ago

Found the mapping from a MediaStreamTrack to an AudioTrack or VideoTrack at W3C Editor's Draft:
http://dev.w3.org/2011/webrtc/editor/getusermedia.html#loading-and-playing-a-mediastream-in-a-media-element

Maybe I should separate the part with DOMMediaStream into another bug?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 55 • 10 years ago

Comment on attachment 8418515 [details] [diff] [review]
Part1: Implement audio/video track API and update TrackEvent

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

This also needs review from a DOM peer.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 56 • 10 years ago

(In reply to Shelly Lin [:shelly] from comment #53)
> - Move the MediaTrackListListener to this patch.
> - I still keep the ability to notify the source media stream when a track
> has enabled or not, but let it be virtual and do nothing in DOMMediaStream,
> since it might mostly overridden by some kind of HWOverlayMediaStream.

It's still the case that setting AudioTrack/VideoTrack.enabled = false on the media element disables the track in the underlying MediaStream. I still think this is a bad idea. I agree it should disable the track, but only for that media element and the MediaStream should be unaffected.

	Shelly Lin [:shelly] Assignee
	Comment 57 • 10 years ago

Comment on attachment 8418517 [details] [diff] [review]
Part2: Enable track list attributes of MediaElement

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3982,5 @@
> +    } else if (VideoTrack* t = mVideoTrackList->GetTrackById(aId)) {
> +      trackID = t->GetTrackID();
> +    }
> +
> +    mSrcStream->NotifyTrackEnabled(trackID, aEnabled);

Hi Roc, I drop the part where it was affecting the enableness of source media stream, so that NotifyTrackEnded() method is now an empty function of DOMMediaStream (please see the diff in DOMMediaStream.h). Make it virtual so that if this source media stream wants to be notified and has NotifyTrackEnabled() overridden.

	Shelly Lin [:shelly] Assignee
	Comment 58 • 10 years ago

Comment on attachment 8418515 [details] [diff] [review]
Part1: Implement audio/video track API and update TrackEvent

Hi bz, roc suggested it needs a review from DOM peers, could you review this patch? Or could you suggest a better fit for the reviewing? Thanks!

	Boris Zbarsky [:bzbarsky]
	Comment 59 • 10 years ago

Comment on attachment 8418515 [details] [diff] [review]
Part1: Implement audio/video track API and update TrackEvent

Where is the one-arg constructor of AudioTrack used?  Is it really needed?  If it is, please make it explicit.

Same for the other one-arg constructors (VideoTrack/MediaTrack).

>+AudioTrack::SetEnabled(bool aEnabled, int aFlags)
>+  NS_ENSURE_TRUE_VOID(mList);

This will warn if !mList.  Why do we want a warning here?

>+++ b/content/media/AudioTrack.h
>+  virtual AudioTrack* AsAudioTrack()

MOZ_OVERRIDE, right?

>+  virtual void SetEnabled(bool aEnabled, int aFlags);

And here.

>+AudioTrackList::operator[](uint32_t aIndex)

It's not really great form to have operator[] that does bounds-tests and can return null.  I would prefer we just guaranteed it returns non-null, with fatal asserts that the access is in-bounds or something.  Or use a method if we want to do the check-and-return-null thing.

Same for MediaTrackList/AudioTrackList.

>+++ b/content/media/AudioTrackList.h 

The operator[] is not actually one of the WebIDL methods.  Probably better to put it (or equivalent, if we use a method instead) before the "WebIDL" comment.

>+++ b/content/media/MediaTrack.h

Should SetTrackList perhaps be protected, with MediaTrackList a friend class?  Either way, I guess.

Why do you need StreamBuffer.h here?

>+class TrackEventRunner : public nsRunnable

Could you please file a followup to add a not-node-specific version of AsyncEventDispatcher?  I assume that's basically what you wanted here....

>+MediaTrackList::GetTrackById(const nsAString& aId)

Wouldn't this avoid some string ops if tracks had a IdIs() method or something?  Or even a GetId() that returns a |const nsString&| to their internal string?

>+MediaTrackList::RemoveTrack(MediaTrack* aTrack)

This function is a bit of a footgun.  Say your caller is not holding a strong reference to aTrack (as in your your part 2 patch).  When you do the RemoveElement, that looks to me like it can call the destructor of aTrack.  And then when you start working with it after that, bad things will happen.

Probably better to take a strong ref to aTrack on the stack here and hold it until the function returns.

>+MediaTrackList::EmptyTracks()

Do we not need to SetTrackList(nullptr) on all the tracks before clearing mTracks?

>+MediaTrackList::DispatchTrackEvent(nsIDOMEvent* aEvent)

Why not have the runnable call DispatchTrustedEvent directly?

>+++ b/content/media/MediaTrackList.h
>+ * VideoTrackList objects represent a dynamic list of zero or more audio and
>+ * video tracks.

Add "respectively" at the end of that comment, so it's clear that VideoTrackList only has video tracks and AudioTrackList only has audio tracks?

>+VideoTrack::SetEnabled(bool aEnabled, int aFlags)

Again, do we really want to warn when !mList?

>+      if ((int32_t)i == curIndex) {

Why not list[i] == this, and then we can skip doing the extra walk in the IndexOf call too.  We could set curIndex here, since we need it later.

>+++ b/content/media/VideoTrack.h
>+  virtual VideoTrack* AsVideoTrack()

MOZ_OVERRIDE.

>+  virtual void SetEnabled(bool aEnabled, int aFlags);

And here.

>+VideoTrackList::EmptyTracks()

Again, need to SetTrackList(nullptr) on the tracks, right?  Probably want to do this by calling the superclass EmptyTracks.

>+++ b/content/media/VideoTrackList.h
>+  virtual void EmptyTracks();

MOZ_OVERRIDE.

r=me with those fixed.

	Shelly Lin [:shelly] Assignee
	Comment 60 • 10 years ago

Carry r+ from roc and bz.

	Shelly Lin [:shelly] Assignee
	Comment 61 • 10 years ago

Comment on attachment 8420797 [details] [diff] [review]
Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

::: content/media/MediaTrackList.h
@@ +38,5 @@
> +  MediaTrack* operator[](uint32_t aIndex);
> +
> +  void AddTrack(MediaTrack* aTrack);
> +
> +  void RemoveTrack(nsRefPtr<MediaTrack>& aTrack);

In patch 2 where this "RemoveTrack" is called by MediaTrackListListener, I've made aTrack a strong reference.
  
nsRefPtr<MediaTrack> track = mMediaTrackList->GetTrackById(aId);
if (track) {
  mMediaTrackList->RemoveTrack(track);
}

Do I still need the above change to the type of pass-in argument?

@@ +47,5 @@
> +
> +  // WebIDL
> +  MediaTrack* IndexedGetter(uint32_t aIndex, bool& aFound);
> +
> +  MediaTrack* GetTrackById(const nsAString& aId);

In reply to:
> Wouldn't this avoid some string ops if tracks had a IdIs() method or something?  Or even a GetId() that returns a |const nsString&| to their internal string?

I'm not sure if I get your idea...can you give me an example of what would go wrong?

	Shelly Lin [:shelly] Assignee
	Comment 62 • 10 years ago

Hi bz, many thanks to your reviews :D

I've addressed most of the issues, but I'm still not clear about the following in comment 61. Please find my questions in above comment, thanks!

	Shelly Lin [:shelly] Assignee
	Comment 63 • 10 years ago

Hi roc, I've remove the parts where an AudioTrack or VideoTrack is affecting (or notifying) the enableness of the track in source media stream. Please see comment 57, where my last patch was just leaving the ability of notifying a source media stream about a track change by its consumer, but I think we can get to that if we ever encounter the situation.

This patch implements the audioTracks and videoTracks interface for media elements, but functions only when it consumes from a MediaStream.

	Boris Zbarsky [:bzbarsky]
	Comment 64 • 10 years ago

> Do I still need the above change to the type of pass-in argument?

I think RemoveTrack should still take on on-stack reference to the incoming value and hold it until the end of the function, yes.  Otherwise whenever someone adds a new caller of RemoveTrack they have to be extra-careful.

> I'm not sure if I get your idea...can you give me an example of what would go wrong?

The current implementation allocates an nsAutoString object, then goes through and does a GetId() on each track, passing in that autostring.  This ends up doing a bunch of atomic stringbuffer refcounting.  I'm proposing we do the following:

1)  Add a |const nsString& GetId() const { return mId; }| to MediaTrack.
2)  Write the loop like so:

  for (uint32_t i = 0; i < Length(); i++) {
    if (aId.Equals(mTracks[i]->GetId())) {
      return mTracks[i];
    }
  }

to avoid the extra string operations.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 65 • 10 years ago

Comment on attachment 8420840 [details] [diff] [review]
Part2: Enable AudioTrack and VideoTrack interfaces for media elements that are consuming a MediaStream

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

OK, this is better :-).

I'm concerned that for media elements that are just playing a regular resource, the track APIs return empty lists. This seems bad, since people would normally use "if (mediaElement.audioTracks)" to test for the audioTracks feature, and now that will succeed but mostly not work.

How much work would it be to implement audioTracks/videoTracks for regular resources as part of this bug?

::: content/media/DOMMediaStream.cpp
@@ +348,5 @@
> +DOMMediaStream::ConstructMediaTracks(AudioTrackList* aAudioTrackList,
> +                                     VideoTrackList* aVideoTrackList)
> +{
> +  int firstEnabledVideo = -1;
> +  for (uint32_t i = 0; i < mTracks.Length(); ++i) {

We don't handle dynamic addition of tracks. Is that a problem?

	Boris Zbarsky [:bzbarsky]
	Comment 66 • 10 years ago

Comment on attachment 8420797 [details] [diff] [review]
Part1: Implement AudioTrack, VideoTrack and other related interfaces

Oh, I see what you did with RemoveTrack.  I guess that's OK, since it does force callers to have the value in an nsRefPtr.  Maybe take a const nsRefPtr&, though?

In VideoTrack::SetEnabled, should curIndex be uint32_t?

The rest looks great.

	Shelly Lin [:shelly] Assignee
	Comment 67 • 10 years ago

Carry r+ from roc and bz.

Thank you very much!! (I feel I've gain another level-up on coding. lol)

	Shelly Lin [:shelly] Assignee
	Comment 68 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #59)
> 
> >+class TrackEventRunner : public nsRunnable
> 
> Could you please file a followup to add a not-node-specific version of
> AsyncEventDispatcher?  I assume that's basically what you wanted here....
> 
Follow up bug for this issue: Bug 1009445 .

	Shelly Lin [:shelly] Assignee
	Comment 69 • 10 years ago

Hey roc, I've make some changes per Part2, could you review this patch again? (I'll comment the changes all together later.)

	Shelly Lin [:shelly] Assignee
	Comment 72 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #65)
> Comment on attachment 8420840 [details] [diff] [review]
> Part2: Enable AudioTrack and VideoTrack interfaces for media elements that
> are consuming a MediaStream
> 
> Review of attachment 8420840 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, this is better :-).
> 
> I'm concerned that for media elements that are just playing a regular
> resource, the track APIs return empty lists. This seems bad, since people
> would normally use "if (mediaElement.audioTracks)" to test for the
> audioTracks feature, and now that will succeed but mostly not work.
> 
> How much work would it be to implement audioTracks/videoTracks for regular
> resources as part of this bug?
> 
Indeed....I've thought of this at the first place too, let me look into it and see how much work it'll take.

One other issue is about the test cases. I've been using the fake MediaStream from gUM and adding some fake attributes like fakeAudioTrackCounts to test multiple tracks, but...the more I tested the more I found the need of changes in gecko, at least we should take TrackID more than audio = 1 and video = 2 (bug 1003695).

	Shelly Lin [:shelly] Assignee
	Comment 73 • 10 years ago

Comment on attachment 8422902 [details] [diff] [review]
Part2: Enable AudioTrack and VideoTrack interfaces for media elements that are consuming a MediaStream

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

::: content/media/DOMMediaStream.cpp
@@ +391,5 @@
> +
> +  int firstEnabledVideo = -1;
> +  for (uint32_t i = 0; i < mTracks.Length(); ++i) {
> +    if (AudioStreamTrack* t = mTracks[i]->AsAudioStreamTrack()) {
> +      nsRefPtr<AudioTrack> track = CreateAudioTrack(t);

Consider avoid writing |track = new AudioTrack(some attributes...)| everywhere, and the ability of letting its derived classes overriding the track creation, I've wrapped the creation of AudioTrack(and VideoTrack) with the protected function |DOMMediaStream::CreateAudioTrack(AudioStreamTrack* aStreamTrack)|. This method then calls the creation function |MediaTrackList::CreateAudioTrack()| from MediaTrackList.

@@ +416,5 @@
> +{
> +  for (uint32_t i = 0; i < mMediaTrackListListeners.Length(); ++i) {
> +    if (AudioStreamTrack* t = aTrack->AsAudioStreamTrack()) {
> +      nsRefPtr<AudioTrack> track = CreateAudioTrack(t);
> +      mMediaTrackListListeners[i].NotifyMediaTrackCreated(track);

Because for now there isn't a way to add a track into a source steam dynamically (or is there?), so I haven't tested it actually, but I think it works theoretically...(I'll find a way to test it!). The below |NotifyMediaStreamTrackEnded()| works as designed.

	Shelly Lin [:shelly] Assignee
	Comment 74 • 10 years ago

Comment on attachment 8422900 [details] [diff] [review]
Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Thank you :)

::: content/media/MediaTrack.cpp
@@ +42,5 @@
> +void
> +MediaTrack::Init(nsPIDOMWindow* aOwnerWindow)
> +{
> +  BindToOwner(aOwnerWindow);
> +  SetIsDOMBinding();

This Init() function is called from MediaTrackList::AddTrack().

Since an AudioTrack or VideoTrack is created by its "source" (DOMMediaStream, in part2) through a static function of MediaTrackList, if we want to keep the previous constructor |MediaTrack::MediaTrack(nsPIDOMWindow* aOwnerWindow, blah....)|, we need to pass in its owner window. Costing small work in |DOMMediaStream::ConstructMediaTracks| but *some* extra work in |DOMMediaStream::NotifyMediaStreamTrackCreated|, so I decide to keep it simple for its creator and move the work here.

(I start to think maybe I should have DOMMediaStream keeping weak references of AudioTrackList and VideoTrackList...)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 75 • 10 years ago

Comment on attachment 8422902 [details] [diff] [review]
Part2: Enable AudioTrack and VideoTrack interfaces for media elements that are consuming a MediaStream

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

Good! However, I think we should try to implement the media-resource media track support at the same time, for the reasons I mentioned.

	Shelly Lin [:shelly] Assignee
	Comment 76 • 10 years ago

Hey Chris,

I'm trying to implement the media-resource with media track supported. Since we only support single audio/video track, my goal here is just to create and add a respectively AudioTrack/VideoTrack to the track list of HTMLMediaElement, during the process of reading metadata. And remove the AudioTrack/VideoTrack from the list when they reach the end.

Here is my WIP patch which adds AudioTrack and VideoTrack during reading a mp4 file, could you give me some feedback on the patch? Or how much work it would actually take for this task (I start to think this is a huge task...)?

p.s. This patch actually gives me a MOZ_CRASH(DOMEventTargetHelper not thread-safe) when it is trying to add the track to a tracklist in MediaDecoder, still haven't figure out a solution.

Thanks!

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 77 • 10 years ago

Comment on attachment 8426953 [details] [diff] [review]
[WIP] Part3: Enable the track interfaces for media element consuming a media resource

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +1904,5 @@
>      mAmpleAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
>      mLowAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
>    }
>  
> +  nsCOMPtr<nsIRunnable> constructMediaTracksEvent =

I think we should instead change AudioMetadataEventRunner to just copy the MediaInfo, and pass that into MediaDecoder::MetadataLoaded(). Then you can call MediaDecoder::ConstructMediaTracks() inside MediaDecoder::MetadataLoaded().

::: content/media/gstreamer/GStreamerReader.cpp
@@ +449,5 @@
>    mInfo.mAudio.mHasAudio = n_audio != 0;
>  
> +  if (n_video > 0) {
> +    mInfo.mVideo.mTrack = new VideoTrack(NS_LITERAL_STRING("1"),
> +                                         NS_LITERAL_STRING("main"),

There are two problems here.

1. VideoTrack is a reference counted object. But here you're storing it into an nsAutoPointer.
2. The MediaDecoderReader::ReadMetadata() functions run on the decode threads. Therefore, here you're creating a dom::VideoTrack on the decode thread, not the main thread. VideoTrack is exposed to JavaScript, so it has main-thread only things in it (like non-threadsafe reference counting, cycle collection), and these are asserting in their constructors because they cannot safely be created or used off the main thread. This is why you see the "MOZ_CRASH(DOMEventTargetHelper not thread-safe)" error.

So I think you should store an nsTArray of info about the tracks here in mInfo.mVideo and mInfo.mAudio, and post that back to the main thread and create the dom::VideoTrack and dom::AudioTrack objects inside MediaDecoder::ConstructMediaTracks().

	Shelly Lin [:shelly] Assignee
	Comment 78 • 10 years ago

Carry r+ from roc.

	Shelly Lin [:shelly] Assignee
	Comment 79 • 10 years ago

Carry r+ from roc.

	Shelly Lin [:shelly] Assignee
	Comment 80 • 10 years ago

I think this patch is good enough to claim our media elements support a basic track interface. It handles addtrack and removetrack events when a media-resource is loaded and ended. Fires a change event when users change the enabled value (but doesn't change the behaviour of playback for now).

I've list out some follow-up TODOs:
1. Set up track info per each codec decoder.
2. Enable/Disable a track should effect the result of playback, e.g. Mute the sound track if users disable the audio track.
3. Expand the ability to handle multiple tracks.

Thank you and the feedback from Chris :D

	Shelly Lin [:shelly] Assignee
	Comment 81 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #77)
> Comment on attachment 8426953 [details] [diff] [review]
> [WIP] Part3: Enable the track interfaces for media element consuming a media
> resource
> 
> Review of attachment 8426953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1904,5 @@
> >      mAmpleAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
> >      mLowAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
> >    }
> >  
> > +  nsCOMPtr<nsIRunnable> constructMediaTracksEvent =
> 
> I think we should instead change AudioMetadataEventRunner to just copy the
> MediaInfo, and pass that into MediaDecoder::MetadataLoaded(). Then you can
> call MediaDecoder::ConstructMediaTracks() inside
> MediaDecoder::MetadataLoaded().
> 
Hmm, I thought of that too, but if I call ConstructMediaTracks() inside MetadataLoaded(), that means if ever a subclass of MediaDecoder or AbstractMediaDecoder overrides the |MetadataLoaded()|, it can not forget calling |ConstructMediaTracks()| too. So consider people forget things easily, I make the call of ConstructMediaTracks() a separated runnable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 82 • 10 years ago

Comment on attachment 8427642 [details] [diff] [review]
Part3: Enable track interfaces for media elements that are consuming a media file

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

Seems reasonable.

However, I think we should support enabling/disabling the audio and video tracks before we ship this. For audio this is pretty easy. You can modify HTMLMediaElement::SetVolumeInternal to set the volume to 0 if the audio track is disabled. Then you just need to call SetVolumeInternal if the audio track is enabled or disabled.

For video, I think HTMLVideoElement::GetVideoSize could check if the video track is selected, and if it's not, return NS_ERROR_FAILURE. That might be good enough. We should have some tests though.

::: content/media/MediaInfo.h
@@ +12,5 @@
>  
>  namespace mozilla {
>  
> +struct TrackInfo {
> +  void Setup(const nsAString& aId,

We usually call this Init().

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 83 • 10 years ago

(In reply to Shelly Lin [:shelly] from comment #81)
> Hmm, I thought of that too, but if I call ConstructMediaTracks() inside
> MetadataLoaded(), that means if ever a subclass of MediaDecoder or
> AbstractMediaDecoder overrides the |MetadataLoaded()|, it can not forget
> calling |ConstructMediaTracks()| too. So consider people forget things
> easily, I make the call of ConstructMediaTracks() a separated runnable.

AudioMetadataEventRunner::MetadataLoaded can call a separate method mDecoder->ConstructMediaTracks(mInfo). So just merge ConstructMediaTracksEventRunner into AudioMetadataEventRunner.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 84 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #83)
> (In reply to Shelly Lin [:shelly] from comment #81)
> > Hmm, I thought of that too, but if I call ConstructMediaTracks() inside
> > MetadataLoaded(), that means if ever a subclass of MediaDecoder or
> > AbstractMediaDecoder overrides the |MetadataLoaded()|, it can not forget
> > calling |ConstructMediaTracks()| too. So consider people forget things
> > easily, I make the call of ConstructMediaTracks() a separated runnable.

Also, you should have a test which would detect if someone forgot to construct the media tracks. ;)

	Shelly Lin [:shelly] Assignee
	Comment 85 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #77)
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1904,5 @@
> >      mAmpleAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
> >      mLowAudioThresholdUsecs /= NO_VIDEO_AMPLE_AUDIO_DIVISOR;
> >    }
> >  
> > +  nsCOMPtr<nsIRunnable> constructMediaTracksEvent =
> 
> I think we should instead change AudioMetadataEventRunner to just copy the
> MediaInfo, and pass that into MediaDecoder::MetadataLoaded(). Then you can
> call MediaDecoder::ConstructMediaTracks() inside
> MediaDecoder::MetadataLoaded().
> 

After a second look of AudioMetadataEventRunner, seems that MediaMetadataManager is dispatching this event runner as well, please see:
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaMetadataManager.h#50

And it is being called by the OggReader, please see:
http://dxr.mozilla.org/mozilla-central/source/content/media/ogg/OggReader.cpp#704

So should I overload the constructor of AudioMetadataEventRunner, and conditionally call the ConstructMediaTracks() in Run(), or would it be simpler to separate them into two event runners?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 86 • 10 years ago

MediaMetadataManager::QueueMetadata() is called when we start playing a new resource appended to the file we're already playing (the next "link" in an Ogg "chain"). The tracks in the resource we're playing change at this point. So I think this is a good time to reevaluate the tracks that we have in the media resource.

So stick to the plan of merging these two events please.

	Shelly Lin [:shelly] Assignee
	Comment 87 • 10 years ago

Carry r+ from roc.

	Shelly Lin [:shelly] Assignee
	Comment 88 • 10 years ago

Hi roc, I've added the part of muting the audio playback if the audio track is set disabled, and disabling the video playback if the video track is un-selected.

Please see |HTMLMediaElement::NotifyMediaTrackEnabled(MediaTrack* aTrack)|.

	Shelly Lin [:shelly] Assignee
	Comment 89 • 10 years ago

Besides enabling the track interface for media-resource, this version also:

- Rename AudioMetadataEventRunner to MetadataEventRunner.
- Call ConstructMediaTracks from MetadataEventRunner.
- For MetadataLoaded() and QueueMetadata(), instead of passing channels, rate, ..., pass MediaInfo at once.

Thank you :)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 90 • 10 years ago

Comment on attachment 8429183 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream

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

Tests please!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 91 • 10 years ago

Comment on attachment 8429186 [details] [diff] [review]
Part3: Enable track interfaces for media elements that are consuming a media file

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

Tests please!

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 92 • 10 years ago

And make sure your tests include a case where you play to the end of media and then seek somewhere and start playing; the active tracks should still be the same.

	Shelly Lin [:shelly] Assignee
	Comment 93 • 10 years ago

Carry r+ from roc and bz.

	Shelly Lin [:shelly] Assignee
	Comment 94 • 10 years ago

Carry r+ from roc and bz (this is the latest version).

	Shelly Lin [:shelly] Assignee
	Comment 95 • 10 years ago

With nit fixed.

Also, remove the previous TrackEventRunner, and use |AsyncEventDispatcher| instead (per comment 59).

	Shelly Lin [:shelly] Assignee
	Comment 96 • 10 years ago

Hi Jason, could you review the part of test case (test_mediatrack_gum.html)? Thanks!

	Jason Smith [:jsmith]
	Comment 97 • 10 years ago

Comment on attachment 8430636 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

I'm really slammed lately with a lot of 2.0 work.

Roc - Can you review this? If not, let me know & I'll try to figure something out.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 98 • 10 years ago

Comment on attachment 8430636 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

::: content/media/test/test_mediatrack_gum.html
@@ +43,5 @@
> +      }
> +
> +      element.videoTracks.onchange = function(e) {
> +        is(element.videoTracks[0].selected, true,
> +           'onchange is fired on videoTracks only when an unselected video track is set selected.');

Setting the video track selected = false should work and we should test that somewhere. I'm surprised that onchange should not fire in that case, where's that in the spec?

If this function should really never be called, just do ok(false) --- it's simpler and clearer than what you're doing here.

@@ +52,5 @@
> +        ok(true, 'onaddtrack is expected to be fired on videoTracks.');
> +        var t = e.track;
> +        for (var i=0; i < videoStreamTracks.length; ++i) {
> +          if (t.id == videoStreamTracks[i].id) {
> +            ok(true, t.id + ' added to the videoTracks.');

We need to add something here to record that onaddtrack fired, so that we can test it later (in loadedmetadata if onaddtrack is guaranteed to fire before that, otherwise in onended) and catch any bugs where onaddtrack didn't fire.

@@ +90,5 @@
> +        if (audioRemovedCount == 0) {
> +          element.audioTracks[0].enabled = false;
> +        }
> +        if (videoRemovedCount == 0) {
> +          element.videoTracks[0].selected = false;

This isn't safe. The track could be removed before this event fires, but the onremovetrack event could still be pending and not run yet. So audioRemovedCount could be 0 but audioTracks.length could be 0 as well.

I think here you should just remove the event listener for canplaythrough to ensure this handler function only runs once. Then you can unconditionally disable the tracks and call stop().

@@ +99,5 @@
> +      element.addEventListener("ended", function() {
> +        is(audioRemovedCount, audioStreamTracks.length,
> +           'Expect to remove the same numbers of audioStreamTrack from audioTracks.');
> +        is(videoRemovedCount, videoStreamTracks.length,
> +           'Expect to remove the same numbers of videoStreamTrack from videoTracks.');

Does the spec actually guarantee that ontrackremoved fires before ended?

	Shelly Lin [:shelly] Assignee
	Comment 99 • 10 years ago

Comment on attachment 8430636 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

::: content/media/test/test_mediatrack_gum.html
@@ +43,5 @@
> +      }
> +
> +      element.videoTracks.onchange = function(e) {
> +        is(element.videoTracks[0].selected, true,
> +           'onchange is fired on videoTracks only when an unselected video track is set selected.');

http://www.w3.org/TR/html5/embedded-content-0.html#dom-videotrack-selected
It is weird of not firing the change event when deselecting a video track, but here is how the spec says:

"Whenever a track in a VideoTrackList that was previously not selected is selected, the user agent must queue a task to fire a simple event named change at the VideoTrackList object. This task must be queued before the task that fires the resize event, if any."

@@ +90,5 @@
> +        if (audioRemovedCount == 0) {
> +          element.audioTracks[0].enabled = false;
> +        }
> +        if (videoRemovedCount == 0) {
> +          element.videoTracks[0].selected = false;

Will do.

@@ +99,5 @@
> +      element.addEventListener("ended", function() {
> +        is(audioRemovedCount, audioStreamTracks.length,
> +           'Expect to remove the same numbers of audioStreamTrack from audioTracks.');
> +        is(videoRemovedCount, videoStreamTracks.length,
> +           'Expect to remove the same numbers of videoStreamTrack from videoTracks.');

Ahh, no it doesn't.

http://www.w3.org/TR/html5/embedded-content-0.html#offsets-into-the-media-resource

"If at any time the user agent learns that an audio or video track has ended and all media data relating to that track corresponds to parts of the media timeline that are before the earliest possible position, the user agent may queue a task to first remove the track from the audioTracks attribute's AudioTrackList object or the videoTracks attribute's VideoTrackList object as appropriate and then fire a trusted event with the name removetrack, that does not bubble and is not cancelable, and that uses the TrackEvent interface, with the track attribute initialized to the AudioTrack or VideoTrack object representing the track, at the media element's aforementioned AudioTrackList or VideoTrackList object."

I'm actually not sure when to fire the removetrack event, currently I'm firing the event at whenever we've found the playback has ended for a track.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 100 • 10 years ago

(In reply to Shelly Lin [:shelly] from comment #99)
> http://www.w3.org/TR/html5/embedded-content-0.html#dom-videotrack-selected
> It is weird of not firing the change event when deselecting a video track,
> but here is how the spec says:
> 
> "Whenever a track in a VideoTrackList that was previously not selected is
> selected, the user agent must queue a task to fire a simple event named
> change at the VideoTrackList object. This task must be queued before the
> task that fires the resize event, if any."

I think it's a mistake in the spec. The change event should fire when any video track's selected status changes. Let's implement it that way and propose for a spec change.

> I'm actually not sure when to fire the removetrack event, currently I'm
> firing the event at whenever we've found the playback has ended for a track.

I think we should. The question is whether these events fire before or after the "ended" event (or whether that's left unspecified). This does mean that for media resources, when we seek we'll need to fire addtrack events.

I'm fine with guaranteeing that they fire before the "ended" event and testing for that, but that should be added to the spec.

That reminds me, we need tests for the media-resource AudioTrack/VideoTrack too.

	Shelly Lin [:shelly] Assignee
	Comment 101 • 10 years ago

I've add a pref for turning on track interface in this patch (and the next).

	Shelly Lin [:shelly] Assignee
	Comment 102 • 10 years ago

Hi roc, could you review the test case in this patch? I've also change the behaviour of firing change event on videoTracks.

Test case of consuming media-resources shall come with part3.

Thank you :)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 103 • 10 years ago

Comment on attachment 8433196 [details] [diff] [review]
Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Looks good. Still needs DOM peer review. Trying Boris since he looked at this a while ago.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 104 • 10 years ago

Comment on attachment 8433200 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

::: content/media/test/test_mediatrack_gum.html
@@ +84,5 @@
> +
> +        var noVideoSelected = true;
> +        for (var i=0; i < element.videoTracks.length; ++i) {
> +          var track = element.videoTracks[i];
> +          if (track.selected == true) {

if (track.selected) {

	Shelly Lin [:shelly] Assignee
	Comment 105 • 10 years ago

This change is per comment 77. I think it would be better to make it a separate patch.

	Boris Zbarsky [:bzbarsky]
	Comment 106 • 10 years ago

I'd appreciate an interdiff against the last thing I looked at, if possible.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 107 • 10 years ago

Comment on attachment 8433995 [details] [diff] [review]
Part 3: Pass MediaInfo to functions that deals with matadata

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

::: content/media/AbstractMediaDecoder.h
@@ +157,4 @@
>      return NS_OK;
>    }
>  
> +  MediaInfo* mInfo;

nsAutoPtr<MediaInfo> since this is an owning reference, I think

	Shelly Lin [:shelly] Assignee
	Comment 108 • 10 years ago

Surprisingly not so many changes since the last solid review. 
The biggest change is using the AsyncEventDispatcher to dispatch events.

	Boris Zbarsky [:bzbarsky]
	Comment 109 • 10 years ago

Comment on attachment 8434657 [details] [diff] [review]
Interdiff of Part 1

Thank you for the interdiff!

>+  nsCOMPtr<EventTarget> target = do_QueryInterface(this);

No need.  |this| inherits from EventTarget, so you can just do:

  nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
    new AsyncEventDispatcher(this, NS_LITERAL_STRING("change"), false);

and same thing for the other AsyncEventDispatcher constructor.

r=me with that

	Boris Zbarsky [:bzbarsky]
	Comment 110 • 10 years ago

Comment on attachment 8433196 [details] [diff] [review]
Part1: Implement AudioTrack, VideoTrack and other related interfaces

r=me based on interdiff.

	Shelly Lin [:shelly] Assignee
	Comment 111 • 10 years ago

Carry r+ from roc and bz.

	Shelly Lin [:shelly] Assignee
	Comment 112 • 10 years ago

I've re-write the test case with Promise, which handles more error cases. Could you take a look at the part of test case? Thanks :)

	Shelly Lin [:shelly] Assignee
	Comment 113 • 10 years ago

Update some comments.

	Shelly Lin [:shelly] Assignee
	Comment 114 • 10 years ago

The ownership of |nsAutoPtr<MediaInfo> mInfo;| in MediaDecoder is transferred from |MetadataEventRunner()|. Sorry that I wasn't clear in the last patch, so I've updated the comment for mInfo in |AbstractMediaDecoder::MetadataEventRunner()| and |MediaMetadataManager::TimedMetadata()|.

Right now it removes all media tracks at once when ever the playback has ended, and reconstructs the media tracks at the start of play if all tracks has previously removed. I'm not sure if tracks should be removed one by one whenever we learn a track has ended or not, if so, it'll be relatively more complicated and I'd like to make that a follow-up bug.

I'll patch a test case for part 3 shortly.

Thank you :)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 115 • 10 years ago

Comment on attachment 8434789 [details] [diff] [review]
Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Nice!

	Shelly Lin [:shelly] Assignee
	Comment 116 • 10 years ago

Test cases done!

One is for testing the regular track events, the other is for testing the addtrack/removetrack when seeking to the end or replay from the end.

Thank you :D

	Shelly Lin [:shelly] Assignee
	Comment 117 • 10 years ago

Carry r+ from roc and bz.

	Shelly Lin [:shelly] Assignee
	Comment 118 • 10 years ago

Carry r+ from roc, fix conflicts due to rebase.

	Shelly Lin [:shelly] Assignee
	Comment 119 • 10 years ago

Carry r+ from roc, fix an error from mochitest "test_chaining.html".

	Shelly Lin [:shelly] Assignee
	Comment 120 • 10 years ago

Hi roc, could you review the part of test cases? I've fixed some errors due to the failure of mochitests, now I'm waiting on the try result, which I think should be in a good shape.

Thanks :)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 121 • 10 years ago

Comment on attachment 8436728 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

::: content/media/test/test_mediatrack_consuming_mediaresource.html
@@ +125,5 @@
> +    element.onloadedmetadata = function(e) {
> +      var t = e.target;
> +      var expectedAudioOnaddtrack = t.test.hasAudio ? 1 : 0;
> +      ok(audioOnaddtrack === expectedAudioOnaddtrack && element.audioTracks.length === expectedAudioOnaddtrack,
> +         'Calls of onaddtrack and the length of audioTracks should be '+expectedAudioOnaddtrack+' times.');

Make these two separate ok() tests.

Shouldn't we have a check that the audio track is selected, if there is one?

@@ +128,5 @@
> +      ok(audioOnaddtrack === expectedAudioOnaddtrack && element.audioTracks.length === expectedAudioOnaddtrack,
> +         'Calls of onaddtrack and the length of audioTracks should be '+expectedAudioOnaddtrack+' times.');
> +      var expectedVideoOnaddtrack = t.test.hasVideo ? 1 : 0;
> +      ok(videoOnaddtrack === expectedVideoOnaddtrack && element.videoTracks.length === expectedVideoOnaddtrack,
> +         'Calls of onaddtrack and the length of videoTracks should be '+expectedVideoOnaddtrack+ 'times.');

Same here. This means a failed test is more informative since we can see which subexpression failed.

@@ +140,5 @@
> +
> +  promise.then(function() {
> +    var p1 = setAudioEnabled(false, 0);
> +    var p2 = setVideoSelected(false, 0);
> +    return Promise.all([p1, p2]);

Seems like we don't have anything to ensure the element hasn't already ended before this function runs (or the function below runs). That seems like a problem, and I'm not sure how to handle it.

Maybe you should pause() the element, wait for "pause", then do all the changes to enabled/disabled so we know tracks don't appear/disappear while we're making those changes, then start playing again.

In fact, there's nothing to guarantee that playback hasn't already ended when onloadedmetata fires. Remember that in theory a playing media element can completely finish before any of the notification events actually get around to firing. So we need to handle that.

@@ +157,5 @@
> +  {
> +    "set": [
> +      ["media.track.enabled", true]
> +    ]
> +  }, manager.runTests(gTrackTests, startTest));

This should be inside function() { ... } I think. Right now runTests gets called before pushPrefEnv.

::: content/media/test/test_mediatrack_replay_from_end.html
@@ +22,5 @@
> +  // 6. All tracks should be added back to the list if we seek from the end
> +  //    to the middle of timeline, and play from that position.
> +
> +  var element = document.createElement("video");
> +  element.src = "short-video.ogv";

use getPlayableVideo(gSmallTests) instead?

@@ +66,5 @@
> +    var audioP1 = new Promise(function(resolve) {
> +      element.audioTracks.addEventListener("addtrack", function onaddtrack() {
> +        element.audioTracks.removeEventListener("addtrack", onaddtrack);
> +        audioOnaddtrackCalls++;
> +        is(element.audioTracks.length, 1, 'Step 1: Length of audioTracks is 1.');

Similar problem to the previous test. The notification events could fire after playback has already finished.

@@ +136,5 @@
> +        element.videoTracks.removeEventListener("removetrack", onremovetrack);
> +        videoOnremovetrackCalls++;
> +        is(element.videoTracks.length, 0, 'Step 5: Length of videoTracks is 0.');
> +        resolve();
> +      });    

trailing whitespace here and above.

@@ +172,5 @@
> +  {
> +    "set": [
> +      ["media.track.enabled", true]
> +    ]
> +  }, startTest);

er, this means the "test" and "token" parameters will be undefined

	Shelly Lin [:shelly] Assignee
	Comment 122 • 10 years ago

Re-base to the latest m-c, carry r+ from roc and bz.

	Shelly Lin [:shelly] Assignee
	Comment 123 • 10 years ago

Rebase to the latest m-c, carry r+ from roc.

	Shelly Lin [:shelly] Assignee
	Comment 124 • 10 years ago

There are some small changes with the MediaDecoderStateMachine since the last review, could you please take a quick look of this patch again, thanks!

	Shelly Lin [:shelly] Assignee
	Comment 125 • 10 years ago

Thanks for the feedback last time :)

I've rewrote the test cases in this patch, though I'm not sure how to handle the case where playback has ended before all events could fire, but I've move most of the actions in onpause call back.

	Shelly Lin [:shelly] Assignee
	Comment 126 • 10 years ago

Sorry, the last patch is slightly outdated.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 127 • 10 years ago

Comment on attachment 8448485 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

::: content/media/test/test_mediatrack_consuming_mediaresource.html
@@ +158,5 @@
> +  promise.then(function() {
> +    element.pause();
> +    var p1 = setAudioEnabled(false, 0);
> +    var p2 = setVideoSelected(false, 0);
> +    return Promise.all([p1, p2]);

If this function runs when the element has already ended, then the element will already be paused so I think no pause event will fire and the test will hang.

Probably the safest thing to do is to stop the test (successfully) if the ended event fires before the setAudioEnabled/setVideoSelected work is done.

@@ +163,5 @@
> +  }).catch(function(err) {
> +    ok(false, 'Something went wrong in onchange callback.');
> +  }).then(function() {
> +    element.play();
> +    element.pause();

A similar problem applied here. The element could already have ended. In that case, play() will restart playback and I'm not sure what pause() will then do.

You should probably call play() then wait for the 'playing' event before calling pause() again.

::: content/media/test/test_mediatrack_replay_from_end.html
@@ +95,5 @@
> +        element.removeEventListener("ended", onended);
> +        Promise.all([promiseOnaddtrackAudio(expectedAddtrackCalls),
> +                     promiseOnaddtrackVideo(expectedAddtrackCalls),
> +                     promiseOnremovetrackAudio(expectedRemovetrackCalls),
> +                     promiseOnremovetrackVideo(expectedRemovetrackCalls)]

Can't you just check videoOnaddtrack etc synchronously? I don't know why you're using Promises here.

@@ +98,5 @@
> +                     promiseOnremovetrackAudio(expectedRemovetrackCalls),
> +                     promiseOnremovetrackVideo(expectedRemovetrackCalls)]
> +        ).then(function() {
> +          resolve();
> +        });

You can just pass "resolve" directly without wrapping it.

@@ +110,5 @@
> +        element.removeEventListener("seeked", onseeked);
> +        Promise.all([promiseOnaddtrackAudio(expectedAddtrackCalls),
> +                     promiseOnaddtrackVideo(expectedAddtrackCalls)]
> +        ).then(function() {
> +          resolve();

same here.

	Shelly Lin [:shelly] Assignee
	Comment 128 • 10 years ago

Looks simpler after removing all the promises..

I've added "finishing test" to handle media is ended before all events could fire in the onended callback.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 129 • 10 years ago

Comment on attachment 8448625 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Looks good, other than the comment below.

::: content/media/test/test_mediatrack_consuming_mediaresource.html
@@ +64,5 @@
> +        is(videoOnremovetrack, 1, 'Calls of onremovetrack on videoTracks should be 1.');
> +        is(element.videoTracks.length, 0, 'The length of videoTracks should be 0.');
> +      }
> +    }
> +    manager.finished(element.token);

In this handler (and the one in the other test), you should remove all event handlers from the element before continuing. Otherwise I think your handlers might keep firing after this, which would be confusing and might cause test failures.

	Shelly Lin [:shelly] Assignee
	Comment 130 • 10 years ago

Ah, yes, thanks for the reminding!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 131 • 10 years ago

Comment on attachment 8449186 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

::: content/media/test/test_mediatrack_consuming_mediaresource.html
@@ +68,5 @@
> +      }
> +    }
> +    element.removeEventListener("ended", checkTrackRemoved);
> +    element.removeEventListener("playing", curOnplayingListener);
> +    element.removeEventListener("pause", curOnpauseListener);

I think it would be simpler to use "onended", "onpause" and "onplaying" so you wouldn't need those extra variables. Instead you could just set
  element.onended = null;
  element.onplaying = null;
  element.onpause = null;

	Shelly Lin [:shelly] Assignee
	Comment 132 • 10 years ago

Oh! I'll keep that in mind next time:)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 133 • 10 years ago

Comment on attachment 8449192 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

::: content/media/test/test_mediatrack_replay_from_end.html
@@ +79,5 @@
> +      finishTesting();
> +    } else if (steps == 1) {
> +      testTrackEventCalls(1);
> +      element.play();
> +      element.addEventListener("playing", onplayingStep2);

You need to stop using addEventListener/removeEventListener and just use the on* attributes, because setting on* to null does not affect any event handlers set using addEventListener.

	Shelly Lin [:shelly] Assignee
	Comment 134 • 10 years ago

Oh!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 135 • 10 years ago

Comment on attachment 8449310 [details] [diff] [review]
Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

::: content/media/test/test_mediatrack_replay_from_end.html
@@ +113,5 @@
> +  manager.started(token);
> +
> +  element.src = test.name;
> +  element.test = test;
> +  element.onplaying = onplaying(1);

This calls onplaying(1) now, which will return "undefined", so not setting element.onplaying to anything :-(.

	Shelly Lin [:shelly] Assignee
	Comment 136 • 10 years ago

Yeah...I figured that too, I'm working on the new version now Orz...

	Shelly Lin [:shelly] Assignee
	Comment 138 • 10 years ago

Rebased to the latest m-c.

	Shelly Lin [:shelly] Assignee
	Comment 139 • 10 years ago

Rebased to the latest m-c.

	Shelly Lin [:shelly] Assignee
	Comment 140 • 10 years ago

Rebased to the latest m-c.

	Shelly Lin [:shelly] Assignee
	Comment 141 • 10 years ago

Carry r+ from roc, rebased to the latest m-c.

	Shelly Lin [:shelly] Assignee
	Comment 142 • 10 years ago

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=6fc5496352ef

\0/!!!

	Carsten Book [:Tomcat]
	Comment 143 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b191be106cae
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8350c756910
https://hg.mozilla.org/integration/mozilla-inbound/rev/7691b13459f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/645a7aeef102

	Carsten Book [:Tomcat]
	Comment 144 • 10 years ago

sorry had to backout this patches for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=43018097&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=43015905&tree=Mozilla-Inbound

	Shelly Lin [:shelly] Assignee
	Comment 148 • 10 years ago

Fix the possible testing errors of test case "content/media/test/test_mediatrack_consuming_mediaresource.html".

Rebase the whole patches to latest m-c.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=4fb8815569b5
https://tbpl.mozilla.org/?tree=Try&rev=7bfb76511604

	Carsten Book [:Tomcat]
	Comment 149 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2ec5485a48
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7b48d0357c
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd17008d9d0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1a4bec4720e

	Tim Taubert [:ttaubert] (inactive)
	Comment 150 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ba2ec5485a48
https://hg.mozilla.org/mozilla-central/rev/6e7b48d0357c
https://hg.mozilla.org/mozilla-central/rev/bd17008d9d0f
https://hg.mozilla.org/mozilla-central/rev/f1a4bec4720e

	micronix
	Comment 151 • 10 years ago

Implementation is wrong ! Needs more fixing.
Open Internet explorer and do not start the video and click on get number of available audio tracks it reports the true length of 2 available audio tracks 

Firefox Nightly latest
reports on the same replication above only 1 audio tracks.
Another issue if the video ends the audiotracks length is zero (0) it should be 2

Please Fix this ASAP

Regards Sascha