Bug 80583 – [Meta] Audio/Video Elements bugs found by the IE test center

• Status: UNCONFIRMED
• Component: Media

[Arun Patole]

Latest IE Testing Center report shows following tests failing on Apple Safari 5.1.2 and Google Chrome 17.0.963.46.
http://samples.msdn.microsoft.com/ietestcenter/#html5MediaElements

Most of them are related to unimplemented AudioTrackList. Need to look at other tests one by one to check if any of them is a bug in our latest implementation and then each can be fixed in a separate bug.

1. HTMLMediaElement's 'audioTracks' attribute is an instance of 'AudioTrackList'.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules

2. There is only ever one 'AudioTrackList' object per 'HTMLMediaElement'.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.1

3. 'AudioTrackList' returns the correct length.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.3

4. 'AudioTrackList' returns the correct length after the video's source has changed.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.4

5. Setting the 'enabled' property of an 'AudioTrack' instance to 'false' disables the track.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.14

6. Setting the 'enabled' property of an 'AudioTrack' instance to 'false' disables the track, and the track becomes inaudible.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.15

7. When a track is enabled, the user agent must queue a task to fire a 'change' event at the 'AudioTrackList' object.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.16

8. When a track is disabled, the user agent must queue a task to fire a 'change' event at the 'AudioTrackList' object.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.17

9. When a track is enabled, the user agent must invoke the function assigned to the 'onchange' property of the 'AudioTrackList' object.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.18

10. When a track is disabled, the user agent must invoke the function assigned to the 'onchange' property of the 'AudioTrackList' object.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.19

11. An 'AudioTrackList' object represents a track list where multiple tracks can be enabled simultaneously.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.26

12. An 'AudioTrackList' object represents a track list where multiple tracks can be disabled simultaneously.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.27

13. Setting the 'enabled' property of an 'AudioTrack' instance to 'true' enables the track.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.32

14. Setting the 'enabled' property of an 'AudioTrack' instance to 'true' enables the track, and the track becomes audible.
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=AudioTrack-rules.33

15. The 'track' property of the 'track' element returns the 'TextTrack' associated with the element 
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=track-rules.1

16. The 'kind' property of the 'TextTrack' object is the 'kind' property of its 'track' element for the value 'subtitles'
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.11

17. The 'language' attribute of a 'TextTrack' object changes to the empty string ("") when the 'language' attribute is removed from its 'track' element
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.27

18. A 'TextTrack' object whose 'track' element has a 'default' attribute becomes showing when that element is appended to a media element that has no default track
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.31

19. A 'TextTrack' object whose 'track' element does not have a 'default' attribute becomes disabled when that element is appended to a media element 
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.33

20. An 'error' event fires if a network error occurs while the track resource is downloaded
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.47

21. Resources for showing 'track' elements are downloaded
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.6

22. Cues in the 'cues' property appear in the correct order
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.9

23. The 'track' property of the 'track' element returns the 'TextTrack' object associated with the element
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.61

24. Cues from showing tracks of kind 'subtitles' are displayed
http://samples.msdn.microsoft.com/ietestcenter/html5/MediaElements_harness.htm?url=TextTrack-rules.64

[Arun Patole]

#1 to #14 - not yet implemented.

#15, #16, #17, #20, PASS on latest chrome after enabling tracks (--enable-video-track)

#18, #19, FAIL on latest chrome after enabling tracks (--enable-video-track)

#21, #22, #23, #24, FAIL, I get:
ASSERTION FAILED: m_textTracks: HTMLMediaElement.cpp(3812) : void WebCore::HTMLMediaElement::configureTextTrackDisplay()

[Arun Patole]

> #21, #22, #23, #24, FAIL, I get:
> ASSERTION FAILED: m_textTracks: HTMLMediaElement.cpp(3812) : void WebCore::HTMLMediaElement::configureTextTrackDisplay()

Is this also happening on mac?
HTMLMediaElement::textTrackModeChanged getting called before HTMLMediaElement::trackWasAdded and so m_textTracks is not assigned any value.

[Victor Carbune]

> #18, #19, FAIL on latest chrome after enabling tracks (--enable-video-track)
#18 can't be passed on WebKit because it has the following test
"if (trackElem.track.mode === TextTrack.OFF)" and TextTrack.OFF is not defined in the spec anymore (or at least I can't find it - seems to be called TextTrack.DISABLED).

[Andrei Poenaru]

Created attachment 133477 [details]
Fixed tests #18 and #19

Added code and relevant test file

[Eric Carlson]

Comment on attachment 133477 [details]
Fixed tests #18 and #19

View in context: https://bugs.webkit.org/attachment.cgi?id=133477&action=review

While this patch fixes the problem, I think it takes the wrong approach. The TextTrackList is just a specialized container that doesn't really know anything about the behavior of a TextTrack. The TextTrackList is owned and managed by an HTMLMediaElement, which already knows about its HTMLTrackElement children, so I think it makes much more sense to have the logic about enabling the default track live there.

Also, I prefer to have the actual spec text in the sources as comments (as we do in most of the media element code). This part of the spec is fairly complex and it is still changing, so having the actual spec in the source makes it easier to follow the logic and to see when we need to change something because of a spec change.

Thanks for taking picking this up!

> Source/WebCore/html/HTMLTrackElement.cpp:84
> +        if (!isDefault())
> +            m_track->setMode(TextTrack::DISABLED, ec);

Are you certain that m_track can't be NULL here? "ensureTrack()->setMode(TextTrack::DISABLED, ec)" would be much safer.

> Source/WebCore/html/track/TextTrackList.h:98
> +    LoadableTextTrack* m_defaultTrack; 

Hanging onto a raw pointer to an object whose lifetime is managed elsewhere is fraught with peril.

> LayoutTests/media/track/track-appended-changes-mode.html:9
> +            var videoElem1, videoElem2;
> +            var trackElem1, trackElem2;

Nit: WebKit style is to declare each variable separately.

> LayoutTests/media/track/track-appended-changes-mode.html:19
> +                testExpected("trackElem1.track.mode == TextTrack.SHOWING", true);

You should also test adding a second track with the default attribute set to make sure it is NOT showing.

> LayoutTests/media/track/track-appended-changes-mode.html:26
> +                run("videoElem2.appendChild(trackElem2)");
> +                testExpected("trackElem2.track.mode == TextTrack.DISABLED", true);

And you might as well test adding a track with 'default' after adding one without to make sure it is enabled.

[Eric Carlson]

Also, I think it would be better to create a new bug for just this fix. We like to have one bug per fix, so consider this the umbrella bug for the changes we need to make to the media element.