Refactor SourceBuffer::AppendData into PrepareAppend and TrackBuffer::AppendData

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

(Blocks 1 bug)

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

4.92 KB, patch
karlt
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
ajones
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
6.02 KB, patch
karlt
: review+
kinetik
: checkin+
Details | Diff | Splinter Review
50.93 KB, patch
Details | Diff | Splinter Review
Assignee

Description

5 years ago
No description provided.
Assignee

Comment 2

5 years ago
Assignee

Comment 4

5 years ago
After the introduction of TrackBuffer, the responsibility was spread strangely between ContainerParser, SourceBuffer, and TrackBuffer.  This set of patches starts to address that by moving code to (or closer to) the appropriate place.
Blocks: MSE
Assignee

Comment 5

5 years ago
Comment on attachment 8486981 [details] [diff] [review]
p2: Remove MP4 MOOV workaround.

I understand the dash-if player bug has been fixed upstream and this workaround should no longer be required.
Attachment #8486981 - Flags: review?(ajones)
Assignee

Updated

5 years ago
Attachment #8486980 - Flags: review?(karlt)
Assignee

Comment 7

5 years ago
Comment on attachment 8486982 [details] [diff] [review]
p3: Factor some of SourceBuffer::AppendData into TrackBuffer::Append.  Move ContainerParser to a new file.

Review of attachment 8486982 [details] [diff] [review]:
-----------------------------------------------------------------

The code moved from SourceBuffer.cpp to ContainerParser.cpp is a pure move, no intentional logic changes.
Attachment #8486982 - Flags: review?(karlt)
Assignee

Comment 9

5 years ago
(In reply to Matthew Gregan [:kinetik] from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=53e85cf90e41

Try or the base rev for that was bad, retried.
Assignee

Comment 11

5 years ago
Attachment #8490495 - Flags: review?(ajones)
Assignee

Updated

5 years ago
Attachment #8486980 - Attachment is obsolete: true
Attachment #8486980 - Flags: review?(karlt)
Assignee

Updated

5 years ago
Attachment #8486981 - Attachment is obsolete: true
Attachment #8486981 - Flags: review?(ajones)
Assignee

Updated

5 years ago
Attachment #8486982 - Attachment is obsolete: true
Attachment #8486982 - Flags: review?(karlt)
Assignee

Updated

5 years ago
Attachment #8490493 - Attachment description: Factor part of SourceBuffer::AppendData into new PrepareAppend function → p1: Factor part of SourceBuffer::AppendData into new PrepareAppend function
Assignee

Updated

5 years ago
Attachment #8490495 - Attachment description: Remove MP4 MOOV workaround → p2: Remove MP4 MOOV workaround
Assignee

Updated

5 years ago
Attachment #8490496 - Attachment description: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file → p3: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file
Assignee

Comment 13

5 years ago
Patched rebased against inbound.
Attachment #8490495 - Flags: review?(ajones) → review+
Assignee

Updated

5 years ago
Attachment #8490495 - Flags: checkin+
Assignee

Updated

5 years ago
Keywords: leave-open
Attachment #8490493 - Flags: review?(karlt) → review+
Comment on attachment 8490496 [details] [diff] [review]
p3: Factor some of SourceBuffer::AppendData into TrackBuffer::Append.  Move ContainerParser to a new file

Looks good.

Can you add MOZ_ASSERT(NS_IsMainThread()); to TrackBuffer::AppendData()
please?  TrackBuffer is used on multiple threads, so I think it is helpful to
document the thread design as clearly as possible.

Can #include "mozilla/Endian.h" be removed from SourceBuffer.cpp now?

Can SourceBuffer::mType be removed?

(In reply to Matthew Gregan [:kinetik] from comment #7)
> The code moved from SourceBuffer.cpp to ContainerParser.cpp is a pure move,
> no intentional logic changes.

but some code changes for mHasInitData, the setting of which has slightly different ordering to that of TrackBuffer::mHasInit.  Because it's all main thread, I suspect there's no real difference.

Please use a stronger value for --find-copies (or hg copy SourceBuffer.cpp
ContainerParser.cpp) so I can see that nothing has changed in
WebMContainerParser and MP4ContainerParser.
Attachment #8490496 - Flags: review?(karlt) → feedback+
Assignee

Comment 18

5 years ago
Assert added, mType and Endian.h (and InitDecoder/DiscardDecoder) removed.  Patch generated with -C30 since -C was not sufficient.
Attachment #8490496 - Attachment is obsolete: true
Attachment #8491214 - Flags: review?(karlt)
Assignee

Updated

5 years ago
Keywords: leave-open
Comment on attachment 8491214 [details] [diff] [review]
p3: v2 Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file

Thank you.
Attachment #8491214 - Flags: review?(karlt) → review+
Assignee

Comment 20

5 years ago
Trivial patch, removes includes and forward declarations that are no longer necessary.  Generated by include-what-you-use, but only taking the simple and obviously correct changes.
Attachment #8491266 - Flags: review?(karlt)
Attachment #8491266 - Flags: review?(karlt) → review+
Depends on: 1069326
Depends on: 1069327
Assignee

Comment 22

5 years ago
Backed out p3: https://hg.mozilla.org/integration/mozilla-inbound/rev/d62a1c36e7c3

Due to Win32 M1 debug OOM weirdness: bug 1069326 and bug 1069327.
No longer depends on: 1069326, 1069327
Keywords: leave-open
Assignee

Updated

5 years ago
Attachment #8490493 - Flags: checkin+
Assignee

Updated

5 years ago
Attachment #8491266 - Flags: checkin+
Depends on: 1069326
Depends on: 1069327
Assignee

Updated

5 years ago
Attachment #8491214 - Attachment is obsolete: true
Assignee

Comment 25

5 years ago
Comment on attachment 8491951 [details] [diff] [review]
p3 v2.1: Factor some of SourceBuffer::AppendData into TrackBuffer::Append.  Move ContainerParser to a new file

Rebased.
Attachment #8491951 - Attachment description: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file → p3 v2.1: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file
Assignee

Updated

5 years ago
Depends on: 1069730
Assignee

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.