1456743 - Add a SourceBuffer::changeType() method

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Jean-Yves Avenard [:jya]]

The issue was first raised in https://github.com/w3c/media-source/issues/155

SourceBuffer::changeType would have the same prototype as MediaSource::addSourceBuffer.

It would allow to change the container type, or the codec type.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1456743 - P1. Create mochitest for new SourceBuffer::changeType.

Review commit: https://reviewboard.mozilla.org/r/240788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240788/

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1456743 - P2. Have MediaFormatReader debug show live audio codec type.

Additionally, show the number of channels and the sampling rate.

Review commit: https://reviewboard.mozilla.org/r/240790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240790/

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

Non functional code.

SourceBuffer.changeType allows to reconfigure a sourceBuffer so that a new codec type or new container format can be fed later.

The new code is placed behind the media.mediasource.experimental.enabled pref.

Review commit: https://reviewboard.mozilla.org/r/240792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240792/

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1456743 - P4. Actual implementation of SourceBuffer.changeType.

Currently, the new init segment provided following a call to changeType() must contain the same number of audio and video tracks as previously.

The Chrome team has indicated concerns in regards to this restriction. TBD.

Review commit: https://reviewboard.mozilla.org/r/240794/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240794/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246534

The WebIDL file should have links to all specs defining things in that file.  Please add that as needed here.  This file doesn't have _any_ such links.  That makes it impossible to do webidl review, since one aspect of such review is checking whether our IDL is following the spec.

::: dom/webidl/SourceBuffer.webidl:49
(Diff revision 1)
>    //void appendStream(Stream stream, [EnforceRange] optional unsigned long long maxSize);
>    [Throws]
>    void abort();
>    [Throws]
>    void remove(double start, unrestricted double end);
> +  [Throws, Func="mozilla::dom::MediaSource::ExperimentalEnabled"]

Why isn't this a [Pref] if that's all you want?  Do you really need the [Func]?  If so why?

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246534

there is no spec yet, currently it is a discussion on the MediaSource github. being able to discuss and contribute to the spec is what this work is about.
being able to present concrete examples of use, knowing the caveats and the potential technical issues.

the links are provided in the bugzilla description.

> Why isn't this a [Pref] if that's all you want?  Do you really need the [Func]?  If so why?

for now...

i followed your previous example for MediaSource which only made it available for a particular web site.

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246534

> for now...
> 
> i followed your previous example for MediaSource which only made it available for a particular web site.

the idea is also to have this function called in another code, which is coming soon

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246566

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246534

> there is no spec yet, currently it is a discussion on the MediaSource github.

I see.  Point to that issue in the .webidl, then.

> the idea is also to have this function called in another code, which is coming soon

OK.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972062 [details]
Bug 1456743 - P1. Create mochitest for new SourceBuffer::changeType.

https://reviewboard.mozilla.org/r/240788/#review246518

::: dom/media/mediasource/test/test_ChangeType.html:14
(Diff revision 1)
> +<body>
> +<pre id="test"><script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();
> +async function setupTest() {
> +  await SpecialPowers.pushPrefEnv({'set': [["media.mediasource.experimental.enabled", true]]});

We're mixing use of single and double quotes for strings, I'd prefer double quotes for all strings as per our linting rules.

::: dom/media/mediasource/test/test_ChangeType.html:26
(Diff revision 1)
> +    // Log events for debugging.
> +    var events = ["suspend", "play", "canplay", "canplaythrough", "loadstart", "loadedmetadata",
> +                  "loadeddata", "playing", "ended", "error", "stalled", "emptied", "abort",
> +                  "waiting", "pause", "durationchange", "seeking", "seeked"];
> +    function logEvent(e) {
> +      var v = e.target;

v is unused

::: dom/media/mediasource/test/test_ChangeType.html:34
(Diff revision 1)
> +    events.forEach(function(e) {
> +      el.addEventListener(e, logEvent);
> +    });
> +
> +    ok(true, "Receive a sourceopen event");
> +        

whitespace

::: dom/media/mediasource/test/test_ChangeType.html:36
(Diff revision 1)
> +    });
> +
> +    ok(true, "Receive a sourceopen event");
> +        
> +    var videosb = ms.addSourceBuffer("video/mp4");
> +    if (!!!videosb.changeType) {

Is there a reason to use `!!!` rather than `!`?

::: dom/media/mediasource/test/test_ChangeType.html:82
(Diff revision 1)
> +      return once(el, 'ended');
> +    })
> +    .then(function() {
> +      ok(el.currentTime >= el.buffered.end(0), "played to the end");
> +      SimpleTest.finish();
> +    })

Missing semicolon

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972063 [details]
Bug 1456743 - P2. Have MediaFormatReader debug show live audio codec type.

https://reviewboard.mozilla.org/r/240790/#review246550

::: dom/media/MediaFormatReader.cpp:3525
(Diff revision 1)
>    if (HasAudio()) {
>      MutexAutoLock lock(mAudio.mMutex);
>      audioDecoderName = mAudio.mDecoder
>                         ? mAudio.mDecoder->GetDescriptionName()
>                         : mAudio.mDescription;
> -    audioType = mInfo.mAudio.mMimeType;
> +    audioType = mAudio.mInfo ? mAudio.mInfo->mMimeType : mInfo.mAudio.mMimeType;

A comment here as to the difference between mAudio.mInfo->mMimeType and mInfo.mAudio.mMimeType would be helpful.

::: dom/media/MediaFormatReader.cpp:3532
(Diff revision 1)
>    if (HasVideo()) {
>      MutexAutoLock mon(mVideo.mMutex);
>      videoDecoderName = mVideo.mDecoder
>                         ? mVideo.mDecoder->GetDescriptionName()
>                         : mVideo.mDescription;
> -    videoType = mInfo.mVideo.mMimeType;
> +    videoType = mVideo.mInfo ? mVideo.mInfo->mMimeType : mInfo.mVideo.mMimeType;

As with the audio case above.

::: dom/media/MediaFormatReader.cpp:3536
(Diff revision 1)
>                         : mVideo.mDescription;
> -    videoType = mInfo.mVideo.mMimeType;
> +    videoType = mVideo.mInfo ? mVideo.mInfo->mMimeType : mInfo.mVideo.mMimeType;
>    }
>  
> -  result += nsPrintfCString(
> -    "Audio Decoder(%s): %s\n", audioType.get(), audioDecoderName.get());
> +  result +=
> +    nsPrintfCString("Audio Decoder(%s, %d channels @ %0.1fkHz): %s\n",

#Channels is always unsgined, can we use an unsgined formatter?

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

https://reviewboard.mozilla.org/r/240792/#review246598

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972065 [details]
Bug 1456743 - P4. Actual implementation of SourceBuffer.changeType.

https://reviewboard.mozilla.org/r/240794/#review246648

::: dom/media/mediasource/TrackBuffersManager.cpp:495
(Diff revision 1)
>      // The demuxer will be recreated during the next run of SegmentParserLoop.
>      // As such we don't need to notify it that data has been removed.
>      mCurrentInputBuffer = new SourceBufferResource();
>    }
>  
>    // We could be left with a demuxer in an unusable state. It needs to be

Consider updating this comment to reflect that we only store the init segement if we haven't changed type.

::: dom/media/mediasource/TrackBuffersManager.cpp:795
(Diff revision 1)
>        }
>        InitializationSegmentReceived();
>        return;
>      }
>      if (mSourceBufferAttributes->GetAppendState() == AppendState::PARSING_MEDIA_SEGMENT) {
>        // 1. If the first initialization segment received flag is false, then run the append error algorithm with the decode error parameter set to true and abort this algorithm.

The language in the spec hasn't yet changed, but our code here is now covering more than just the case discussed in the comment. Consider amending the comment.

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972063 [details]
Bug 1456743 - P2. Have MediaFormatReader debug show live audio codec type.

https://reviewboard.mozilla.org/r/240790/#review246550

> A comment here as to the difference between mAudio.mInfo->mMimeType and mInfo.mAudio.mMimeType would be helpful.

it's used in many places througout the code already...

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972065 [details]
Bug 1456743 - P4. Actual implementation of SourceBuffer.changeType.

https://reviewboard.mozilla.org/r/240794/#review246648

> Consider updating this comment to reflect that we only store the init segement if we haven't changed type.

that's not what this code does...
It initialise the ContainerParser with init segment. But we don't have an init segment yet, it's will come with the next appendBuffer

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972062 [details]
Bug 1456743 - P1. Create mochitest for new SourceBuffer::changeType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240788/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972063 [details]
Bug 1456743 - P2. Have MediaFormatReader debug show live audio codec type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240790/diff/1-2/

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972064 [details]
Bug 1456743 - P3. Add SourceBuffer.changeType experimental method.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240792/diff/1-2/

Comment 19

[Jean-Yves Avenard [:jya]]

Comment on attachment 8972065 [details]
Bug 1456743 - P4. Actual implementation of SourceBuffer.changeType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240794/diff/1-2/

Comment 20

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972065 [details]
Bug 1456743 - P4. Actual implementation of SourceBuffer.changeType.

https://reviewboard.mozilla.org/r/240794/#review246648

> that's not what this code does...
> It initialise the ContainerParser with init segment. But we don't have an init segment yet, it's will come with the next appendBuffer

Isn't that the same thing? If we've changed types we don't have the init segment to store?

Comment 21

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8972063 [details]
Bug 1456743 - P2. Have MediaFormatReader debug show live audio codec type.

https://reviewboard.mozilla.org/r/240790/#review246550

> it's used in many places througout the code already...

Right, but that doesn't make the difference between the mime type of your info's audio or your audio's info clear to those unfamilar with the code.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f4214755cd7
P1. Create mochitest for new SourceBuffer::changeType. r=bryce
https://hg.mozilla.org/integration/autoland/rev/c526527f499e
P2. Have MediaFormatReader debug show live audio codec type. r=bryce
https://hg.mozilla.org/integration/autoland/rev/d89660b4978f
P3. Add SourceBuffer.changeType experimental method. r=bryce,bz
https://hg.mozilla.org/integration/autoland/rev/d858e46ec74a
P4. Actual implementation of SourceBuffer.changeType. r=bryce

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0f4214755cd7
https://hg.mozilla.org/mozilla-central/rev/c526527f499e
https://hg.mozilla.org/mozilla-central/rev/d89660b4978f
https://hg.mozilla.org/mozilla-central/rev/d858e46ec74a