Abstract SourceBuffer usage of TrackBuffer

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Aim is to allow easily replacing the existing TrackBuffer implementation with another one.
(Assignee)

Comment 1

4 years ago
Created attachment 8607344 [details] [diff] [review]
Abstract TrackBuffer interface

Abstract TrackBuffer into a SourceBufferContentManager.
Attachment #8607344 - Flags: review?(ajones)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
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?
Attachment #8607344 - Flags: review?(ajones) → review+
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 8611612 [details] [diff] [review]
Part2. Use TimeUnits in SourceBuffer

:mattwoodrow
(Assignee)

Comment 5

4 years ago
Created attachment 8614483 [details] [diff] [review]
Part2. Use TimeUnits in SourceBuffer

Part2. Use TimeUnits in SourceBuffer
Attachment #8614483 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Attachment #8611612 - Attachment is obsolete: true
Attachment #8614483 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/ab20361ff7d3
https://hg.mozilla.org/mozilla-central/rev/24d733651b48
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
You need to log in before you can comment on or make changes to this bug.