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

RESOLVED FIXED in mozilla35

Status

()

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
Comment hidden (empty)
(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: 778617
(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
Last Resolved: 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.