Bug 110798 – Factor SourceBuffer methods out of MediaSourcePrivate & WebMediaSource into SourceBufferPrivate & WebSourceBuffer respectively.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Aaron Colwell]

SourceBuffer functionality is currently routed through the MediaSourcePrivate & WebMediaSource interfaces. This functionality should be moved out into new SourceBufferPrivate and WebSourceBuffer interfaces so that it is easier to add SourceBuffer functionality without having to modify the MediaSource classes.

[Aaron Colwell]

abarth@ please take a look at this patch. It is the SourceBuffer changes that you asked me to place in a seperate patch when you were reviewing Bug 109857.

[Aaron Colwell]

Created attachment 190128 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 190128 [details]
Patch

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

I don't feel 100% confident in reviewing this patch, but it seems correct.  Should we get someone who knows the media code well to take a look as well?

> Source/WebKit/chromium/src/SourceBufferPrivateImpl.cpp:42
> +SourceBufferPrivateImpl::SourceBufferPrivateImpl(WTF::PassOwnPtr<WebSourceBuffer> sourceBuffer)

WTF::  <-- No need for this prefix.

> Source/WebKit/chromium/src/SourceBufferPrivateImpl.h:44
> +    explicit SourceBufferPrivateImpl(WTF::PassOwnPtr<WebSourceBuffer>);

ditto

> Source/WebKit/chromium/src/SourceBufferPrivateImpl.h:55
> +    WTF::OwnPtr<WebSourceBuffer> m_sourceBuffer;

ditto

[Aaron Colwell]

Thanks for the review abarth@.

jer.noble@ or eric.carlson@ could one of you take a peek at this for me please.

[Aaron Colwell]

Comment on attachment 190128 [details]
Patch

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

>> Source/WebKit/chromium/src/SourceBufferPrivateImpl.cpp:42
>> +SourceBufferPrivateImpl::SourceBufferPrivateImpl(WTF::PassOwnPtr<WebSourceBuffer> sourceBuffer)
> 
> WTF::  <-- No need for this prefix.

done.

>> Source/WebKit/chromium/src/SourceBufferPrivateImpl.h:44
>> +    explicit SourceBufferPrivateImpl(WTF::PassOwnPtr<WebSourceBuffer>);
> 
> ditto

done.

>> Source/WebKit/chromium/src/SourceBufferPrivateImpl.h:55
>> +    WTF::OwnPtr<WebSourceBuffer> m_sourceBuffer;
> 
> ditto

done.

[Aaron Colwell]

Created attachment 190361 [details]
Patch

[Jer Noble]

Comment on attachment 190361 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:64
> -    if (!m_source) {
> +    if (isClosed()) {
>          ec = INVALID_STATE_ERR;
>          return 0;
>      }

This is incorrect, as far as  the spec goes.  This shouldn't return an error if the MediaSource is closed, but only if the buffer has been removed from the media element.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:88
> -    if (!m_source) {
> +    if (isClosed()) {
>          ec = INVALID_STATE_ERR;
>          return;
>      }
>  
> -    if (m_source->setTimestampOffset(id(), offset, ec))
> -        m_timestampOffset = offset;
> +    if (m_source->readyState() == MediaSource::endedKeyword())
> +        m_source->setReadyState(MediaSource::openKeyword());
> +
> +    if (!m_private->setTimestampOffset(offset)) {
> +        ec = INVALID_STATE_ERR;
> +        return;
> +    }
> +    m_timestampOffset = offset;

As someone who is not very familiar with the MediaSource extensions spec, this is all somewhat confusing.  But looking at the current spec for SourceBuffer <https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#sourcebuffer>, it appears you're not checking the "updating" attribute here.  If that is checked elsewhere, it would be helpful to note that here.

What I've done in other specced areas of the code is to include the text of the spec as comments. I know the WebKit community is generally anti-comment, but I believe it would help here.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:98
> +    if (!data.get()) {

The .get() should be unnecessary here.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:109
> +    if (m_source->readyState() == MediaSource::endedKeyword())
> +        m_source->setReadyState(MediaSource::openKeyword());
> +
> +    if (!m_private->append(data->data(), data->length())) {
> +        ec = SYNTAX_ERR;
> +        return;
> +    }

Again, you don't seem to be checking the "updating" attribute.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:114
> +    if (!m_private || !m_source || m_source->readyState() != MediaSource::openKeyword()) {

m_private should never be NULL, due to the ASSERT in MediaSource::addSourceBuffer(). You may want to add an ASSERT in the constructor to guarantee this. This would require not clearing m_private in clear().

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:124
> +    if (m_private)

Ditto.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:125
> +        m_private->removedFromMediaSource();

Why is this not named m_private->clear()?  Or contrariwise, why isn't the name of this method SourceBuffer::removedFromMediaSource()?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:127
> +    m_private.clear();

Why clear your private pointer here? Is it expected that the private implementation is holding onto resources?

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:132
> +bool SourceBuffer::isClosed() const
> +{
> +    return !m_private || !m_source || m_source->readyState() == MediaSource::closedKeyword();

Again, if you don't clear your m_private, you don't need to check it here.  The !m_source check would be sufficient.

I also think you're conflating the "closed" state and the "removed from source" state.  The spec seems to make a distinction, and it may be worthwhile to add a "isRemoved()" method that simply checks "!m_source".

If you do want to keep the concept of clearing the private implementation, you should provide inline wrappers for the various states you're checking (isClosed(), isRemoved(), isOpen()) so as to avoid repeating "!m_private || !m_source ||..." everywhere.

> Source/WebCore/platform/graphics/MediaSourcePrivate.h:35
> +#include <wtf/text/WTFString.h>

Since you seem to be only passing around the String& type, and forward declaration of String would suffice here. You may need to rename all the String& references to WTF::String&.

[Aaron Colwell]

Comment on attachment 190361 [details]
Patch

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

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:64
>>      }
> 
> This is incorrect, as far as  the spec goes.  This shouldn't return an error if the MediaSource is closed, but only if the buffer has been removed from the media element.

Changed to isRemoved() to be more consistent w/ the spec text. This code was simply relying on the !m_source test in isClosed(). I also just realized that isRemoved() check overrides the "closed" checks in other parts of the spec so I'll be removing those from the spec.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:88
>> +    m_timestampOffset = offset;
> 
> As someone who is not very familiar with the MediaSource extensions spec, this is all somewhat confusing.  But looking at the current spec for SourceBuffer <https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html#sourcebuffer>, it appears you're not checking the "updating" attribute here.  If that is checked elsewhere, it would be helpful to note that here.
> 
> What I've done in other specced areas of the code is to include the text of the spec as comments. I know the WebKit community is generally anti-comment, but I believe it would help here.

The current WebKit implementation doesn't support the updating attribute yet. The updating attribute and async append & remove operations are relatively new additions to the spec. WebKit essentially implements the October 1st version of the spec  (https://dvcs.w3.org/hg/html-media/raw-file/7bab66368f2c/media-source/media-source.html) right now which doesn't have updating.

This refactor is part of the process to update the implementation to the current spec. The addition of updating & related logic will be added in a followup patch soon.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:98
>> +    if (!data.get()) {
> 
> The .get() should be unnecessary here.

Done

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:109
>> +    }
> 
> Again, you don't seem to be checking the "updating" attribute.

Updating is not implement yet. It will be added in a followup patch.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:114
>> +    if (!m_private || !m_source || m_source->readyState() != MediaSource::openKeyword()) {
> 
> m_private should never be NULL, due to the ASSERT in MediaSource::addSourceBuffer(). You may want to add an ASSERT in the constructor to guarantee this. This would require not clearing m_private in clear().

Done

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:124
>> +    if (m_private)
> 
> Ditto.

Done

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:127
>> +    m_private.clear();
> 
> Why clear your private pointer here? Is it expected that the private implementation is holding onto resources?

That was the initial thinking. The private implementation likely has a handle to all the media data in the SourceBuffer. This could be purged on the removedFromMediaSource() call though so I'll just remove this clear() call.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:132
>> +    return !m_private || !m_source || m_source->readyState() == MediaSource::closedKeyword();
> 
> Again, if you don't clear your m_private, you don't need to check it here.  The !m_source check would be sufficient.
> 
> I also think you're conflating the "closed" state and the "removed from source" state.  The spec seems to make a distinction, and it may be worthwhile to add a "isRemoved()" method that simply checks "!m_source".
> 
> If you do want to keep the concept of clearing the private implementation, you should provide inline wrappers for the various states you're checking (isClosed(), isRemoved(), isOpen()) so as to avoid repeating "!m_private || !m_source ||..." everywhere.

You are right. I've added isRemoved() and noticed that most of the "closed" checks in the SourceBuffer algorithms actually are redundant because isRemoved() should always be true if the source is "closed" because all SourceBuffers are removed when the source transtions to "closed". I'll be removing these "closed" checks in the next spec update.

>> Source/WebCore/platform/graphics/MediaSourcePrivate.h:35
>> +#include <wtf/text/WTFString.h>
> 
> Since you seem to be only passing around the String& type, and forward declaration of String would suffice here. You may need to rename all the String& references to WTF::String&.

I'd prefer not specifying WTF::String since similar interfaces in MediaPlayerPrivate.h don't do this. I tried using #include<wtf/Forward.h> like MediaPlayerPrivate.h does, but this also required that I #include <wtf/Vector.h> as well to make the compiler happy. It isn't clear if this was better than just including <wtf/text/WTFString.h> here.

[Aaron Colwell]

Created attachment 190606 [details]
Patch

[Jer Noble]

Comment on attachment 190606 [details]
Patch

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

This is much better.  I really appreciate the link to the specific version of the spec matching the implementation. Just one nit:

> Source/WebCore/platform/graphics/MediaSourcePrivate.h:36
> +#include <wtf/Forward.h>
> +#include <wtf/Vector.h>

Nit: it shouldn't be necessary to include <wtf/Vector.h> here as you're only using a const Vector<String>& in the header. I believe you might be getting the compile error below...:

> Source/WebCore/platform/graphics/MediaSourcePrivate.h:48
> +    virtual AddStatus addSourceBuffer(const String& type, const Vector<String>& codecs, OwnPtr<SourceBufferPrivate>*) = 0;

...because Forward.h doesn't define the default value for the second template parameter for Vector.  Change this to a "const Vector<String, 0>&", and I think that will fix the compile error when you remove the #include <wtf/Vector.h>.

That said, this is just a nit.

[Aaron Colwell]

Created attachment 190750 [details]
Fix remaining nit.

[Aaron Colwell]

Comment on attachment 190606 [details]
Patch

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

Thanks Jer for the quick an thorough review. :)

>> Source/WebCore/platform/graphics/MediaSourcePrivate.h:48
>> +    virtual AddStatus addSourceBuffer(const String& type, const Vector<String>& codecs, OwnPtr<SourceBufferPrivate>*) = 0;
> 
> ...because Forward.h doesn't define the default value for the second template parameter for Vector.  Change this to a "const Vector<String, 0>&", and I think that will fix the compile error when you remove the #include <wtf/Vector.h>.
> 
> That said, this is just a nit.

Fixed. I removed the #include<wtf/Vector.h> and added a CodecsArray typedef similar to what I've seen in other files so that I don't have to copy the default parameter everywhere.

[WebKit Review Bot]

Comment on attachment 190750 [details]
Fix remaining nit.

Clearing flags on attachment: 190750

Committed r144328: <http://trac.webkit.org/changeset/144328>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.