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

Peng Liu
Reported 2020-11-28 20:54:52 PST
[MSE] Move track buffer handling from SourceBuffer to SourceBufferPrivate
Attachments
WIP patch (193.23 KB, patch)
2020-11-28 20:59 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP patch (193.85 KB, patch)
2020-11-28 21:37 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP patch (195.89 KB, patch)
2020-11-29 15:22 PST, Peng Liu 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
WIP patch (195.77 KB, patch)
2020-11-29 16:54 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP patch (195.83 KB, patch)
2020-11-29 19:57 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix layout test failures (195.92 KB, patch)
2020-11-30 09:58 PST, Peng Liu 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
WIP: fix layout test failures (198.98 KB, patch)
2020-11-30 11:59 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (196.30 KB, patch)
2020-11-30 13:24 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: rebase the patch (195.29 KB, patch)
2020-11-30 13:44 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (195.74 KB, patch)
2020-11-30 14:03 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (195.75 KB, patch)
2020-11-30 16:45 PST, Peng Liu 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (195.75 KB, patch)
2020-11-30 16:51 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (195.73 KB, patch)
2020-11-30 17:03 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
WIP: fix build failures on WPE and GTK ports (196.24 KB, patch)
2020-11-30 17:18 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
Patch (195.52 KB, patch)
2020-11-30 20:12 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
Add change logs (209.61 KB, patch)
2020-12-01 10:55 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
rebase the patch (209.39 KB, patch)
2020-12-01 18:54 PST, Peng Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (16) View All Add attachment proposed patch, testcase, etc.
Peng Liu
Comment 1 2020-11-28 20:59:10 PST Comment hidden (obsolete)
Peng Liu
Comment 2 2020-11-28 21:37:52 PST Comment hidden (obsolete)
Peng Liu
Comment 3 2020-11-29 15:22:25 PST Comment hidden (obsolete)
Peng Liu
Comment 4 2020-11-29 16:54:33 PST Comment hidden (obsolete)
Peng Liu
Comment 5 2020-11-29 19:57:38 PST Comment hidden (obsolete)
Peng Liu
Comment 6 2020-11-30 09:58:00 PST Comment hidden (obsolete)
Peng Liu
Comment 7 2020-11-30 11:59:41 PST Comment hidden (obsolete)
Peng Liu
Comment 8 2020-11-30 13:24:54 PST Comment hidden (obsolete)
Peng Liu
Comment 9 2020-11-30 13:44:55 PST Comment hidden (obsolete)
Peng Liu
Comment 10 2020-11-30 14:03:26 PST Comment hidden (obsolete)
Peng Liu
Comment 11 2020-11-30 16:45:58 PST Comment hidden (obsolete)
Peng Liu
Comment 12 2020-11-30 16:51:17 PST Comment hidden (obsolete)
Peng Liu
Comment 13 2020-11-30 17:03:41 PST Comment hidden (obsolete)
Peng Liu
Comment 14 2020-11-30 17:18:41 PST Comment hidden (obsolete)
Peng Liu
Comment 15 2020-11-30 20:12:28 PST
Created attachment 415090 [details] Patch
Philippe Normand
Comment 16 2020-12-01 05:25:29 PST
Can you explain the reason(s) for this patch in the ChangeLog please? :)
Peng Liu
Comment 17 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.
Radar WebKit Bug Importer
Comment 18 2020-12-01 10:02:17 PST
<rdar://problem/71855115>
Peng Liu
Comment 19 2020-12-01 10:55:17 PST
Created attachment 415149 [details] Add change logs
Peng Liu
Comment 20 2020-12-01 18:54:08 PST
Created attachment 415187 [details] rebase the patch
Daniel Bates
Comment 21 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.
EWS
Comment 22 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].
Peng Liu
Comment 23 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.
Note You need to log in before you can comment on or make changes to this bug.