RESOLVED FIXED 219332
[MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate
https://bugs.webkit.org/show_bug.cgi?id=219332
Summary [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate
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
WIP patch (193.85 KB, patch)
2020-11-28 21:37 PST, Peng Liu
no flags
WIP patch (195.89 KB, patch)
2020-11-29 15:22 PST, Peng Liu
ews-feeder: commit-queue-
WIP patch (195.77 KB, patch)
2020-11-29 16:54 PST, Peng Liu
no flags
WIP patch (195.83 KB, patch)
2020-11-29 19:57 PST, Peng Liu
no flags
WIP: fix layout test failures (195.92 KB, patch)
2020-11-30 09:58 PST, Peng Liu
ews-feeder: commit-queue-
WIP: fix layout test failures (198.98 KB, patch)
2020-11-30 11:59 PST, Peng Liu
no flags
WIP: fix build failures on WPE and GTK ports (196.30 KB, patch)
2020-11-30 13:24 PST, Peng Liu
no flags
WIP: rebase the patch (195.29 KB, patch)
2020-11-30 13:44 PST, Peng Liu
no flags
WIP: fix build failures on WPE and GTK ports (195.74 KB, patch)
2020-11-30 14:03 PST, Peng Liu
no flags
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-
WIP: fix build failures on WPE and GTK ports (195.75 KB, patch)
2020-11-30 16:51 PST, Peng Liu
no flags
WIP: fix build failures on WPE and GTK ports (195.73 KB, patch)
2020-11-30 17:03 PST, Peng Liu
no flags
WIP: fix build failures on WPE and GTK ports (196.24 KB, patch)
2020-11-30 17:18 PST, Peng Liu
no flags
Patch (195.52 KB, patch)
2020-11-30 20:12 PST, Peng Liu
no flags
Add change logs (209.61 KB, patch)
2020-12-01 10:55 PST, Peng Liu
no flags
rebase the patch (209.39 KB, patch)
2020-12-01 18:54 PST, Peng Liu
no flags
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
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
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.