Bug 219332 – [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate

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

[Peng Liu]

[MSE] Move track buffer handling from SourceBuffer to SourceBufferPrivate

[Peng Liu]

Created attachment 414986 [details]
WIP patch

[Peng Liu]

Created attachment 414988 [details]
WIP patch

[Peng Liu]

Created attachment 414994 [details]
WIP patch

[Peng Liu]

Created attachment 414999 [details]
WIP patch

[Peng Liu]

Created attachment 415001 [details]
WIP patch

[Peng Liu]

Created attachment 415030 [details]
WIP: fix layout test failures

[Peng Liu]

Created attachment 415040 [details]
WIP: fix layout test failures

[Peng Liu]

Created attachment 415049 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415051 [details]
WIP: rebase the patch

[Peng Liu]

Created attachment 415053 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415076 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415078 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415080 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415082 [details]
WIP: fix build failures on WPE and GTK ports

[Peng Liu]

Created attachment 415090 [details]
Patch

[Philippe Normand]

Can you explain the reason(s) for this patch in the ChangeLog please? :)

[Peng Liu]

(In reply to Philippe Normand from comment #16)
> Can you explain the reason(s) for this patch in the ChangeLog please? :)

Sure. This patch is a preparation to run MSE media player in the GPU Process. I will add the change logs after fixing layout test failures.

[Radar WebKit Bug Importer]

<rdar://problem/71855115>

[Peng Liu]

Created attachment 415149 [details]
Add change logs

[Peng Liu]

Created attachment 415187 [details]
rebase the patch

[Daniel Bates]

Comment on attachment 415187 [details]
rebase the patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:106
> +    m_private->setIsAttached(false);

Consider flipping the order of these statements to be inverse of constructor order to be idiomatic: construct A, B, destroy B, A.

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:181
> +        return Vector<String>();

Ok as is. Could return {}

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:183
> +    TrackBuffer& trackBuffer = it->value;

Ok as-is. Could use auto.

> Source/WebCore/platform/graphics/SourceBufferPrivate.h:84
> +    TimeRanges* buffered() const { return m_buffered.get(); }

Consider making non-const if possible. General observation: encapsulation not violated only if either const function returns const data or by value. Returning non const data from const method still allows caller to mutate data out from under this object.

[EWS]

Committed r270435: <https://trac.webkit.org/changeset/270435>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415187 [details].

[Peng Liu]

Comment on attachment 415187 [details]
rebase the patch

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

Thanks for the review! I will include the fixes in the patch for bug #219402.

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:106
>> +    m_private->setIsAttached(false);
> 
> Consider flipping the order of these statements to be inverse of constructor order to be idiomatic: construct A, B, destroy B, A.

Good point. Will include the fix in the patch for bug#219402.

>> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:181
>> +        return Vector<String>();
> 
> Ok as is. Could return {}

Right. Will fix it in the patch for bug #219402.

>> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:183
>> +    TrackBuffer& trackBuffer = it->value;
> 
> Ok as-is. Could use auto.

Right.

>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:84
>> +    TimeRanges* buffered() const { return m_buffered.get(); }
> 
> Consider making non-const if possible. General observation: encapsulation not violated only if either const function returns const data or by value. Returning non const data from const method still allows caller to mutate data out from under this object.

Good point! Will refactor the related functions in the patch for bug #219402.