Closed Bug 1065250 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: kinetik, Assigned: kinetik)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

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
No description provided.
Attached patch p2: Remove MP4 MOOV workaround. (obsolete) — Splinter Review
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
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)
Attachment #8486980 - Flags: review?(karlt)
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)
(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.
Attachment #8490495 - Flags: review?(ajones)
Attachment #8486980 - Attachment is obsolete: true
Attachment #8486980 - Flags: review?(karlt)
Attachment #8486981 - Attachment is obsolete: true
Attachment #8486981 - Flags: review?(ajones)
Attachment #8486982 - Attachment is obsolete: true
Attachment #8486982 - Flags: review?(karlt)
Attachment #8490493 - Attachment description: Factor part of SourceBuffer::AppendData into new PrepareAppend function → p1: Factor part of SourceBuffer::AppendData into new PrepareAppend function
Attachment #8490495 - Attachment description: Remove MP4 MOOV workaround → p2: Remove MP4 MOOV workaround
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
Patched rebased against inbound.
Attachment #8490495 - Flags: review?(ajones) → review+
Attachment #8490495 - Flags: checkin+
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+
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)
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+
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
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
Attachment #8490493 - Flags: checkin+
Attachment #8491266 - Flags: checkin+
Depends on: 1069326
Depends on: 1069327
Attachment #8491214 - Attachment is obsolete: true
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
Depends on: 1069730
Status: ASSIGNED → RESOLVED
Closed: 6 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.