Summary: | [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||||||||||||||||||||||||||||||||
Component: | Media | Assignee: | 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
Peng Liu
2020-11-28 20:54:52 PST
Created attachment 414986 [details]
WIP patch
Created attachment 414988 [details]
WIP patch
Created attachment 414994 [details]
WIP patch
Created attachment 414999 [details]
WIP patch
Created attachment 415001 [details]
WIP patch
Created attachment 415030 [details]
WIP: fix layout test failures
Created attachment 415040 [details]
WIP: fix layout test failures
Created attachment 415049 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415051 [details]
WIP: rebase the patch
Created attachment 415053 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415076 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415078 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415080 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415082 [details]
WIP: fix build failures on WPE and GTK ports
Created attachment 415090 [details]
Patch
Can you explain the reason(s) for this patch in the ChangeLog please? :) (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. Created attachment 415149 [details]
Add change logs
Created attachment 415187 [details]
rebase the patch
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. Committed r270435: <https://trac.webkit.org/changeset/270435> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415187 [details]. 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. |