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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: p1: Factor part of SourceBuffer::AppendData into new PrepareAppend function.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: p2: Remove MP4 MOOV workaround.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: p3: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file.

Comment 4

[Matthew Gregan [:kinetik]]

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.

Comment 5

[Matthew Gregan [:kinetik]]

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.

Comment 6

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=53e85cf90e41

Comment 7

[Matthew Gregan [:kinetik]]

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.

Comment 8

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=151e3696242d

Comment 9

[Matthew Gregan [:kinetik]]

(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.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: p1: Factor part of SourceBuffer::AppendData into new PrepareAppend function

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: p2: Remove MP4 MOOV workaround

Comment 12

[Matthew Gregan [:kinetik]]

Attachment: p3: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file

Comment 13

[Matthew Gregan [:kinetik]]

Patched rebased against inbound.

Comment 14

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd132d164dc9

Comment 15

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e88aeebca097

Comment 16

[Karl Tomlinson (:karlt)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd132d164dc9
https://hg.mozilla.org/mozilla-central/rev/e88aeebca097

Comment 18

[Matthew Gregan [:kinetik]]

Attachment: p3: v2 Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file

Assert added, mType and Endian.h (and InitDecoder/DiscardDecoder) removed.  Patch generated with -C30 since -C was not sufficient.

Comment 19

[Karl Tomlinson (:karlt)]

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.

Comment 20

[Matthew Gregan [:kinetik]]

Attachment: p4: Remove unneeded includes.

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.

Comment 21

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b533b3527076
https://hg.mozilla.org/integration/mozilla-inbound/rev/783379123376
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a508290d5f8

Comment 22

[Matthew Gregan [:kinetik]]

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

Due to Win32 M1 debug OOM weirdness: bug 1069326 and bug 1069327.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b533b3527076
https://hg.mozilla.org/mozilla-central/rev/0a508290d5f8

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: p3 v2.1: Factor some of SourceBuffer::AppendData into TrackBuffer::Append. Move ContainerParser to a new file

Comment 25

[Matthew Gregan [:kinetik]]

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.

Comment 26

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=0996a8f6731a

Comment 27

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17d48ba5979b

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17d48ba5979b