1165819 - Abstract SourceBuffer usage of TrackBuffer

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

Aim is to allow easily replacing the existing TrackBuffer implementation with another one.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Abstract TrackBuffer interface

Abstract TrackBuffer into a SourceBufferContentManager.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8607344 [details] [diff] [review]
Abstract TrackBuffer interface

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

Excellent!

::: dom/media/mediasource/SourceBufferContentManager.h
@@ +47,5 @@
> +  enum class EvictDataResult : int8_t
> +  {
> +    NO_DATA_EVICTED,
> +    DATA_EVICTED,
> +    CANT_EVICT,

Did we evict? http://thedailywtf.com/articles/What_Is_Truth_0x3f_

::: dom/media/mediasource/TrackBuffer.h
@@ +33,5 @@
>    // Append data to the current decoder.  Also responsible for calling
>    // NotifyDataArrived on the decoder to keep buffered range computation up
>    // to date.  Returns false if the append failed.
> +  nsRefPtr<AppendPromise> AppendData(MediaLargeByteBuffer* aData,
> +                                     int64_t aTimestampOffset /* microseconds */) override;

Are we still using MOZ_OVERRIDE or is that a thing of the past?

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #2)
> Comment on attachment 8607344 [details] [diff] [review]
> Abstract TrackBuffer interface
> 
> Review of attachment 8607344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Excellent!
> 
> ::: dom/media/mediasource/SourceBufferContentManager.h
> @@ +47,5 @@
> > +  enum class EvictDataResult : int8_t
> > +  {
> > +    NO_DATA_EVICTED,
> > +    DATA_EVICTED,
> > +    CANT_EVICT,
> 
> Did we evict? http://thedailywtf.com/articles/What_Is_Truth_0x3f_

LOL.

We need to know if we didn't evict for a particular reason. As I didn't want to abstract some method that are unique to the current implementation of TrackBuffer, it's the best I could come up with.

> 
> ::: dom/media/mediasource/TrackBuffer.h
> @@ +33,5 @@
> >    // Append data to the current decoder.  Also responsible for calling
> >    // NotifyDataArrived on the decoder to keep buffered range computation up
> >    // to date.  Returns false if the append failed.
> > +  nsRefPtr<AppendPromise> AppendData(MediaLargeByteBuffer* aData,
> > +                                     int64_t aTimestampOffset /* microseconds */) override;
> 
> Are we still using MOZ_OVERRIDE or is that a thing of the past?

yes... Now that all supported compiler are C++11 compliant, we can use override.
And there's a new static analysis in place that if a member is marked as override, it can't be marked as virtual also as it's implicit with just override.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Use TimeUnits in SourceBuffer

:mattwoodrow

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Part2. Use TimeUnits in SourceBuffer

Part2. Use TimeUnits in SourceBuffer

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab20361ff7d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d733651b48

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ab20361ff7d3
https://hg.mozilla.org/mozilla-central/rev/24d733651b48

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8607344 [details] [diff] [review]
Abstract TrackBuffer interface

>+++ b/dom/media/mediasource/SourceBufferContentManager.h
>+class SourceBufferContentManager {
[...]
>+#if defined(DEBUG)
>+  virtual void Dump(const char* aPath) { }
>+#endif
[...]
>+++ b/dom/media/mediasource/TrackBuffer.h
>-class TrackBuffer final {
>+class TrackBuffer final : public SourceBufferContentManager {
[...]
> #if defined(DEBUG)
>   void Dump(const char* aPath);
> #endif

This ^ function, TrackBuffer::Dump, is missing an "override" keyword (since this changeset implicitly promoted it to be overriding a function in a newly-created interface, as shown in my snippet above).

This causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+).

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a657a4840aee

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/a657a4840aee