744896 - [Stingray] Support HTMLMediaElement.audioTracks and videoTracks

Status: VERIFIED FIXED

(Core :: Audio/Video, enhancement)

Description

[Ralph Giles (:rillian)]

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.

Comment 1

[Ralph Giles (:rillian)]

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.

Comment 3

[Brendan Long]

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?

Comment 4

[Andrew Quartey [:drexler]]

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

Comment 5

[Shelly Lin [:shelly]]

Hi Brendan, 
Is there any update on the implementation?

Comment 6

[Brendan Long]

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.

Comment 7

[Brendan Long]

Attachment: Attempt to add audio and video track API, but has compile errors

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.

Comment 8

[Andrew Quartey [:drexler]]

> 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

Comment 9

[Brendan Long]

(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;
};

Comment 10

[Brendan Long]

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

Comment 11

[Brendan Long]

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.

Comment 12

[Brendan Long]

Attachment: Add basic audio and video track API

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.

Comment 13

[Brendan Long]

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.

Comment 14

[Brendan Long]

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

Comment 15

[Shelly Lin [:shelly]]

Thank you Brendan!

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

Comment 16

[Shelly Lin [:shelly]]

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 :)?

Comment 17

[Brendan Long]

(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

Comment 18

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API

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?

Comment 19

[Shelly Lin [:shelly]]

Attachment: Part2: Create TrackEvent

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You will need a DOM peer to review these patches.

Comment 23

[Shelly Lin [:shelly]]

(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 :)

Comment 24

[Shelly Lin [:shelly]]

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?

Comment 25

[Shelly Lin [:shelly]]

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!

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Shelly Lin [:shelly]]

Attachment: Part 3: Add track list to HTMLMediaElement [WIP]

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Shelly Lin [:shelly]]

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

Comment 31

[Shelly Lin [:shelly]]

(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 :)

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

Comment 33

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API and update TrackEvent

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?

Comment 34

[Shelly Lin [:shelly]]

Attachment: Part 3: Add track list to HTMLMediaElement [WIP]

Please find my comment inline.

Comment 35

[Shelly Lin [:shelly]]

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?

Comment 36

[Shelly Lin [:shelly]]

(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:)

Comment 37

[Boris Zbarsky [:bzbarsky]]

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.

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

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

Comment 40

[Boris Zbarsky [:bzbarsky]]

I filed bug 1000944 to make the union bit work.

Comment 41

[Shelly Lin [:shelly]]

(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 :)

Comment 42

[Shelly Lin [:shelly]]

Attachment: bug-744896-p1-add-av-tracks.patch

Comment 43

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API and update TrackEvent

Comment 44

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API and update TrackEvent

A more refined version.

Comment 45

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track list attributes of MediaElement

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

Comment 46

[Shelly Lin [:shelly]]

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.

Comment 47

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API and update TrackEvent

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

Comment 48

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track list attributes of MediaElement

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

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 51

[Marco Chen [:mchen]]

(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

Comment 52

[Shelly Lin [:shelly]]

Attachment: Part1: Implement audio/video track API and update TrackEvent

Address feedback and update some comments.

Comment 53

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track list attributes of MediaElement

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

Comment 54

[Shelly Lin [:shelly]]

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?

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 57

[Shelly Lin [:shelly]]

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.

Comment 58

[Shelly Lin [:shelly]]

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!

Comment 59

[Boris Zbarsky [:bzbarsky]]

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.

Comment 60

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc and bz.

Comment 61

[Shelly Lin [:shelly]]

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?

Comment 62

[Shelly Lin [:shelly]]

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!

Comment 63

[Shelly Lin [:shelly]]

Attachment: Part2: Enable AudioTrack and VideoTrack interfaces for media elements that are consuming a MediaStream

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.

Comment 64

[Boris Zbarsky [:bzbarsky]]

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

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 66

[Boris Zbarsky [:bzbarsky]]

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.

Comment 67

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc and bz.

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

Comment 68

[Shelly Lin [:shelly]]

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

Comment 69

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Comment 70

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Comment 71

[Shelly Lin [:shelly]]

Attachment: Part2: Enable AudioTrack and VideoTrack interfaces for media elements that are consuming a MediaStream

Comment 72

[Shelly Lin [:shelly]]

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

Comment 73

[Shelly Lin [:shelly]]

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.

Comment 74

[Shelly Lin [:shelly]]

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

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 76

[Shelly Lin [:shelly]]

Attachment: [WIP] Part3: Enable the track interfaces for media element consuming a media resource

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!

Comment 77

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 78

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc.

Comment 79

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream

Carry r+ from roc.

Comment 80

[Shelly Lin [:shelly]]

Attachment: Part3: Enable track interfaces for media elements that are consuming a media file

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

Comment 81

[Shelly Lin [:shelly]]

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

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 84

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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. ;)

Comment 85

[Shelly Lin [:shelly]]

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

Comment 86

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 87

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc.

Comment 88

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream

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

Comment 89

[Shelly Lin [:shelly]]

Attachment: Part3: Enable track interfaces for media elements that are consuming a media file

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

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 91

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 92

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 93

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc and bz.

Comment 94

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Comment 95

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

With nit fixed.

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

Comment 96

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Comment 97

[Jason Smith [:jsmith]]

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.

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 99

[Shelly Lin [:shelly]]

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.

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 101

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Comment 102

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Comment 103

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 105

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that deals with matadata

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

Comment 106

[Boris Zbarsky [:bzbarsky]]

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

Comment 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 108

[Shelly Lin [:shelly]]

Attachment: Interdiff of Part 1

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

Comment 109

[Boris Zbarsky [:bzbarsky]]

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

Comment 110

[Boris Zbarsky [:bzbarsky]]

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

r=me based on interdiff.

Comment 111

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc and bz.

Comment 112

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Comment 113

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that deals with matadata

Update some comments.

Comment 114

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource

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

Comment 115

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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!

Comment 116

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Comment 117

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Carry r+ from roc and bz.

Comment 118

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Comment 119

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that deals with matadata

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

Comment 120

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Comment 121

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 122

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

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

Comment 123

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

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

Comment 124

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that handles matadata

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

Comment 125

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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.

Comment 126

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

Sorry, the last patch is slightly outdated.

Comment 127

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 128

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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.

Comment 129

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 130

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

Ah, yes, thanks for the reminding!

Comment 131

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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;

Comment 132

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 134

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

Oh!

Comment 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 136

[Shelly Lin [:shelly]]

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

Comment 137

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

Comment 138

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Rebased to the latest m-c.

Comment 139

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream

Rebased to the latest m-c.

Comment 140

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that deals with matadata

Rebased to the latest m-c.

Comment 141

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Comment 142

[Shelly Lin [:shelly]]

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

\0/!!!

Comment 143

[Carsten Book [:Tomcat]]

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

Comment 144

[Carsten Book [:Tomcat]]

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

Comment 145

[Shelly Lin [:shelly]]

Attachment: Part1: Implement AudioTrack, VideoTrack and other related interfaces

Comment 146

[Shelly Lin [:shelly]]

Attachment: Part2: Enable track interfaces for media elements that are consuming a MediaStream (w/ test case)

Comment 147

[Shelly Lin [:shelly]]

Attachment: Part 3: Pass MediaInfo to functions that deals with matadata

Comment 148

[Shelly Lin [:shelly]]

Attachment: Part4: Enable track interfaces for media elements that are consuming media-resource (w/ test case)

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

Comment 149

[Carsten Book [:Tomcat]]

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

Comment 150

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 151

[micronix]

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

Comment 152

[micronix]

Also shows the Internet Explorer 11 the two letter code in the AudioTrackList FireFox Nightly not

<--Build Config-->
 
Build Machine

B-2008-IX-0169
Source

Built from https://hg.mozilla.org/mozilla-central/rev/c45f6e6d6005

Comment 153

[Boris Zbarsky [:bzbarsky]]

[e-mail], please file a separate bug on your issue, with a link to the testcase showing the problem.

Comment 154

[Marco Chen [:mchen]]

(In reply to micronix from comment #151)
> Please Fix this ASAP
> 
> Regards Sascha

Hi micronix,

It is good to hear someone tried to use this Web API.

But this bug tried to implement functions on the Web API level only.
Actually it didn't cover to support each codecs reported real tracks information into Web API.
And the follow up bug is [1].

[1] Bug 1022524 - Set up track information for AudioTrack and VideoTrack per different codec decoder

Comment 155

[Randell Jesup [:jesup] (needinfo me)]

We should update https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement to indicate it will be in 33

Comment 156

[Shelly Lin [:shelly]]

Done updating the MDN page.

Comment 157

[Jean-Yves Perrier [:teoli]]

Thank you Shelly. I've also added a note in https://developer.mozilla.org/en-US/Firefox/Releases/33

The pref is off by default (even for Nightly), is this correct?

Comment 158

[Shelly Lin [:shelly]]

Yes, I'd like to pref it on after landing bug 1022524. Thank you :).

Comment 159

[Jason Smith [:jsmith]]

No end-user QA verification needed, so marking verified.

Comment 160

[Stuart Pinfold]

Hi, we are working on a project which involves multi-language audio files.

We have an MP4 file with four encoded audio tracks. You can download this file at http://www.stuart-pinfold.co.uk/videotest/test.mp4 - play it in VLC, right-click and go to Audio > Audio Track, and you can change language.

Compare this page in Internet Explorer (version 10 or 11 only) with even the latest version of Firefox:
http://www.stuart-pinfold.co.uk/videotest/

IN INTERNET EXPLORER:
- The four links to change language all work
- There is even a button in the HTML5 player itself to switch audio tracks

IN FIREFOX:
- Clicking immediately to play the video gives you English
- Clicking any of the languages results in silence and even clicking English does not re-activate the first audio track (English).
- There is no audio-selection tool in the video, even in the context menu.

Is this just a feature that Firefox hasn't yet implemented (surprising, given IE10's release date in 2012) or does Firefox interpret this video differently and thus our code needs to be changed?

Any help or advice appreciated.

Comment 161

[Stuart Pinfold]

Version 35.0 nightly beta release - still no sign of audioTracks working according to the W3C spec.

Something has changed since my last message in October, as now Firefox sticks with the first (English) track rather than going silent and never recovering, even when English is re-selected.

Test page:
http://www.stuart-pinfold.co.uk/videotest/

Any advice appreciated!

Stuart

Comment 162

[Shelly Lin [:shelly]]

Hi Stuart,

Please file a new bug on your issue, thank you.

Comment 163

[Shelly Lin [:shelly]]

(In reply to Stuart Pinfold from comment #161)
> Version 35.0 nightly beta release - still no sign of audioTracks working
> according to the W3C spec.
> 
> Something has changed since my last message in October, as now Firefox
> sticks with the first (English) track rather than going silent and never
> recovering, even when English is re-selected.
> 
> Test page:
> http://www.stuart-pinfold.co.uk/videotest/
> 
> Any advice appreciated!
> 
> Stuart

Did you turn on the preference of media.track.enabled in about:config?

Comment 164

[Stuart Pinfold]

Hi Shelly, happy new year!

I did turn on media.track.enabled - *before* my message in October.

Why is a new bug report required? This has never worked, at least not using the same code as our test in IE which *does* work perfectly, yet this bug is marked as "verified fixed". It is not, and never has been - unless we're doing something wrong. Please do take a look at the test link in both Firefox and IE and you will see the difference.

Stuart

Comment 165

[Boris Zbarsky [:bzbarsky]]

> Why is a new bug report required?

Because generally we have a policy of bug per checkin.  Otherwise there's a lot of confusion about what got checked in where and tracking things becomes impossible.  For example, after a checking QA is supposed to verify the fix.  That means reading the bug, and this bug already has 164 comments in it...

> This has never worked

There are clearly cases in which it's working (e.g. the ones tested by the automated tests checked in for this bug, though looking at them carefully they never seem to test _multiple_ tracks!).  Your issue is not that the DOM API is not supported at all, but that some parts of the support are incorrect or missing.  That really is a separate issue from "not supported at all" in terms of what we need to do with it, though clearly quite similar for your purposes.

Again, no one is saying you're not seeing a problem or that we don't want to fix it or anything like that.  Shelly is telling you that the most effective way to get it fixed is to file a new bug, with either an attached testcase or a link to the testcase.

I've gone ahead and done that for you: bug 1119299.

Comment 166

[jonobr1]

I'm on Firefox 41.0.2 on Mac OS X. The Mozilla site says audioTracks are implemented as of FF 33: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/audioTracks

They don't seem to exist on my browser. Is that specific to me or is that webpage incorrect? Thanks for the help!

Comment 167

[Boris Zbarsky [:bzbarsky]]

See bug 1119299 comment 3.  It's implemented behind a pref but not turned on yet.