MSE: Implement TrackDefault object

Source/modules/mediasource/TrackDefault.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "modules/mediasource/TrackDefault.h"
+
+// BIG TODO other includes...

[philipj_slow, 2014/10/31 11:48:10]

What other includes? Much to my surprise, this patch compiles as is :)

[wolenetz, 2014/11/01 00:24:02]

Hah! This was a note-to-self I forgot to remove. Thanks! :)
Though patch set 1 compiles, it doesn't include the new code in compilation.
(Source/modules/modules.gypi needed updating, too).

+
+namespace blink {
+
+const AtomicString& TrackDefault::audioKeyword()
+{
+    DEFINE_STATIC_LOCAL(const AtomicString, audio, ("audio", AtomicString::ConstructFromLiteral));
+    return audio;
+}
+
+const AtomicString& TrackDefault::videoKeyword()
+{
+    DEFINE_STATIC_LOCAL(const AtomicString, video, ("video", AtomicString::ConstructFromLiteral));
+    return video;
+}
+
+const AtomicString& TrackDefault::textKeyword()
+{
+    DEFINE_STATIC_LOCAL(const AtomicString, text, ("text", AtomicString::ConstructFromLiteral));
+    return text;
+}
+
+TrackDefault::TrackDefault(const AtomicString& type, const String& language, const String& label, const Vector<String>& kinds, const String& byteStreamTrackID, ExceptionState& exceptionState)
+{
+    // Per 06 Oct 2014 Editor's Draft
+    // https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#idl-def-TrackDefault
+    // When this method is invoked, the user agent must run the following steps:
+    // 1. if |language| is not an empty string or |language| is not a BCP 47

[philipj_slow, 2014/10/31 11:48:10]

Spec issue: I don't think throwing here is a good idea, and e.g. addTextTrack
doesn't throw for an invalid language:
https://html.spec.whatwg.org/#dom-media-addtexttrack

[acolwell GONE FROM CHROMIUM, 2014/10/31 19:14:30]

It appears that the HTML spec is underspecified in this case then. The
definition of the language
attribute(https://html.spec.whatwg.org/#text-track-language) is to be a BCP47
language tag. If in invalid tag is passed in, ISTM an exception or at least some
sort of behavior to force a valid language tag value should be specified. MSE is
just throwing an exception to prevent invalid language tags from getting in so
that we don't expose values that violate what the HTML spec says the attribute
should contain.

[philipj_slow, 2014/10/31 19:55:16]

Yes, HTML does say "This is a string (a BCP 47 language tag) representing the
language of the text track's cues." but it doesn't do anything to enforce it
when the language comes from <track>. A validator would complain though.

Practically speaking, I'm worried about the amount of testing needed to reach
interop on this exception and that there may not be existing code to check if a
string matches the langtag production in the spec. Blink doesn't seem to have
it, since grepping for "i-default" yields nothing.

[wolenetz, 2014/11/01 00:24:02]

I've asked acolwell@ separately to take a look at your question here. I would
rather we land something that matches spec, so the init segment received
algorithm implementation doesn't become delayed.

+    //    language tag, then throw an INVALID_ACCESS_ERR and abort these steps.
+    // FIXME: Implement BCP 47 language tag validation, and confirm spec logic
+    // for this step should be s/or/and/.
+
+    if (type == audioKeyword()) {
+        // 2.1. If |type| equals "audio":
+        //      If any string in |kinds| contains a value that is not listed as
+        //      applying to audio in the kind categories table, then throw an
+        //      INVALID_ACCESS_ERR and abort these steps.

[philipj_slow, 2014/10/31 11:48:10]

Spec issue: INVALID_ACCESS_ERR is a bit odd I think. I think it'd be cleaner
with AudioTrackKind and VideoTrackKind enums used in the interfaces, but it's a
problem that we have a TrackDefault for all three track kinds, so that won't
actually work. Similar to invalid kind on <track>, it would be less work to
simply treat an invalid kind as "" on the eventual AudioTrack/VideoTrack object.
Are you planning on writing that code in the CL?

[acolwell GONE FROM CHROMIUM, 2014/10/31 19:14:30]

I don't particularly agree with this. I think it makes more sense to explicitly
signal that an invalid value was passed in instead of hiding an application
coding error by simply converting it to "". I think the implicit conversion
would be more surprising than getting an exception telling them that they tried
to use an invalid value. 

Perhaps throwing a TypeError instead might be a little more appropriate since
that is what would happen if this method actually had enum parameter and an
invalid value was passed.

[philipj_slow, 2014/10/31 19:55:16]

Sure, TypeError would be better, and in line with what addTextTrack does when
you try a bogus kind.

[wolenetz, 2014/11/01 00:24:02]

The code in this CL will only be the TrackDefault object (and layout tests).
Subsequent CLs will cover TrackDefaultList, SourceBuffer.TrackDefaults, and then
the init segment received algorithm. If we find spec blockers in this process,
we can bring them up with acolwell@ (who is Google Chrome's MSE spec editor)
and/or file w3c MSE spec bugs.

ditto: I've also asked acolwell@ separately to take a look at your spec question
here. I would rather we land something that matches spec, so the init segment
received algorithm implementation doesn't become delayed.

+        for (int i = 0; i < kinds.length(); ++i) {

[philipj_slow, 2014/10/31 11:48:10]

for (const String& kind : kinds) should work.

[wolenetz, 2014/11/01 00:24:02]

Patch set 2 includes reference to i now. Otherwise, I agree.

+            if (!AudioTrack::isValidKindKeyword(kinds[i])) {
+                exceptionState.throwDOMException(InvalidAccessError, "Invalid audio track default kind");
+                return;
+            }
+        }
+    } else if (type == videoKeyword()) {
+        // 2.2. If |type| equals "video":
+        //      If any string in |kinds| contains a value that is not listed as
+        //      applying to video in the kind categories table, then throw an
+        //      INVALID_ACCESS_ERR and abort these steps.
+        for (int i = 0; i < kinds.length(); ++i) {
+            if (!VideoTrack::isValidKindKeyword(kinds[i])) {
+                exceptionState.throwDOMException(InvalidAccessError, "Invalid video track default kind");
+                return;
+            }
+        }
+    } else {
+        ASSERT(type == textKeyword());
+        // 2.3. If |type| equals "text":
+        //      If any string in |kinds| contains a value that is not listed in the
+        //      text track kind list, then throw an INVALID_ACCESS_ERR and abort
+        //      these steps.
+        for (int i = 0; i < kinds.length(); ++i) {
+            if (!TextTrack::isValidKindKeyword(kinds[i])) {
+                exceptionState.throwDOMException(InvalidAccessError, "Invalid text track default kind");
+                return;
+            }
+        }
+    }
+
+    // 3. Set the type attribute on this new object to |type|.
+    m_type = type;
+
+    // 4. Set the language attribute on this new object to |language|.
+    m_language = language;
+
+    // 5. Set the label attribute on this new object to |label|.
+    m_label = label;
+
+    // 6. Set the kinds attribute on this new object to |kinds|.
+    m_kinds = kinds;
+
+    // 7. Set the byteStreamTrackID attribute on this new object to
+    //    |byteStreamTrackID|.
+    m_byteStreamTrackID = byteStreamTrackID;

[philipj_slow, 2014/10/31 11:48:10]

I'd be interested to see how this ends up being used in the end.

[wolenetz, 2014/11/01 00:24:02]

There could be adjustments in later CLs (especially if the Chromium stream
parsers aren't coherently exposing track id info). My goal is to follow the MSE
spec's init segment received algorithm, with as much as possible implemented in
Blink for interop.

+}
+
+} // namespace blink