Bug 219332 - [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate
Summary: [MSE] Move track buffer management from SourceBuffer to SourceBufferPrivate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks: 219402
  Show dependency tree / graph
 
Reported: 2020-11-28 20:54 PST by Peng Liu
Modified: 2020-12-04 21:39 PST (History)
16 users (show)

See Also:

Attachments
WIP patch (193.23 KB, patch)
2020-11-28 20:59 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (193.85 KB, patch)
2020-11-28 21:37 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (195.89 KB, patch)
2020-11-29 15:22 PST, Peng Liu 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (195.77 KB, patch)
2020-11-29 16:54 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (195.83 KB, patch)
2020-11-29 19:57 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: fix layout test failures (195.92 KB, patch)
2020-11-30 09:58 PST, Peng Liu 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP: fix layout test failures (198.98 KB, patch)
2020-11-30 11:59 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: fix build failures on WPE and GTK ports (196.30 KB, patch)
2020-11-30 13:24 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: rebase the patch (195.29 KB, patch)
2020-11-30 13:44 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: fix build failures on WPE and GTK ports (195.74 KB, patch)
2020-11-30 14:03 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
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-
Details | Formatted Diff | Diff
WIP: fix build failures on WPE and GTK ports (195.75 KB, patch)
2020-11-30 16:51 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: fix build failures on WPE and GTK ports (195.73 KB, patch)
2020-11-30 17:03 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP: fix build failures on WPE and GTK ports (196.24 KB, patch)
2020-11-30 17:18 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
Patch (195.52 KB, patch)
2020-11-30 20:12 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
Add change logs (209.61 KB, patch)
2020-12-01 10:55 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
rebase the patch (209.39 KB, patch)
2020-12-01 18:54 PST, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (16) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.