Bug 219332

Summary: [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate
Product: WebKit Reporter: Peng Liu <peng.liu6>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gustavo, gyuyoung.kim, jer.noble, menard, philipj, pnormand, ryuan.choi, sergio, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=219565
Bug Depends on:    
Bug Blocks: 219402    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
ews-feeder: commit-queue-
WIP patch
none
WIP patch
none
WIP: fix layout test failures
ews-feeder: commit-queue-
WIP: fix layout test failures
none
WIP: fix build failures on WPE and GTK ports
none
WIP: rebase the patch
none
WIP: fix build failures on WPE and GTK ports
none
WIP: fix build failures on WPE and GTK ports
ews-feeder: commit-queue-
WIP: fix build failures on WPE and GTK ports
none
WIP: fix build failures on WPE and GTK ports
none
WIP: fix build failures on WPE and GTK ports
none
Patch
none
Add change logs
none
rebase the patch none

Description Peng Liu 2020-11-28 20:54:52 PST 
[MSE] Move track buffer handling from SourceBuffer to SourceBufferPrivate
Comment 1 Peng Liu 2020-11-28 20:59:10 PST Comment hidden (obsolete)
Comment 2 Peng Liu 2020-11-28 21:37:52 PST Comment hidden (obsolete)
Comment 3 Peng Liu 2020-11-29 15:22:25 PST Comment hidden (obsolete)
Comment 4 Peng Liu 2020-11-29 16:54:33 PST Comment hidden (obsolete)
Comment 5 Peng Liu 2020-11-29 19:57:38 PST Comment hidden (obsolete)
Comment 6 Peng Liu 2020-11-30 09:58:00 PST Comment hidden (obsolete)
Comment 7 Peng Liu 2020-11-30 11:59:41 PST Comment hidden (obsolete)
Comment 8 Peng Liu 2020-11-30 13:24:54 PST Comment hidden (obsolete)
Comment 9 Peng Liu 2020-11-30 13:44:55 PST Comment hidden (obsolete)
Comment 10 Peng Liu 2020-11-30 14:03:26 PST Comment hidden (obsolete)
Comment 11 Peng Liu 2020-11-30 16:45:58 PST Comment hidden (obsolete)
Comment 12 Peng Liu 2020-11-30 16:51:17 PST Comment hidden (obsolete)
Comment 13 Peng Liu 2020-11-30 17:03:41 PST Comment hidden (obsolete)
Comment 14 Peng Liu 2020-11-30 17:18:41 PST Comment hidden (obsolete)
Comment 15 Peng Liu 2020-11-30 20:12:28 PST 
Created attachment 415090 [details]
Patch
Comment 16 Philippe Normand 2020-12-01 05:25:29 PST 
Can you explain the reason(s) for this patch in the ChangeLog please? :)
Comment 17 Peng Liu 2020-12-01 09:06:25 PST 
(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.
Comment 18 Radar WebKit Bug Importer 2020-12-01 10:02:17 PST 
<rdar://problem/71855115>
Comment 19 Peng Liu 2020-12-01 10:55:17 PST 
Created attachment 415149 [details]
Add change logs
Comment 20 Peng Liu 2020-12-01 18:54:08 PST 
Created attachment 415187 [details]
rebase the patch
Comment 21 Daniel Bates 2020-12-01 19:18:42 PST 
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.
Comment 22 EWS 2020-12-04 10:12:45 PST 
Committed r270435: <https://trac.webkit.org/changeset/270435>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415187 [details].
Comment 23 Peng Liu 2020-12-04 10:18:38 PST 
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.