Closed Bug 1378631 Opened 4 years ago Closed 4 years ago

Remove AbstractMediaDecoder::CanonicalDurationOrNull()

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Assignee: nobody → jwwang
Blocks: 1378295
Priority: -- → P3
Attachment #8884177 - Flags: review?(jyavenard)
Attachment #8884178 - Flags: review?(jyavenard)
Attachment #8884179 - Flags: review?(jyavenard)
Comment on attachment 8884177 [details]
Bug 1378631. P1 - move the duration mirror from MediaDecoderReader to MediaDecoderReaderWrapper.

https://reviewboard.mozilla.org/r/155098/#review160168

::: dom/media/MediaDecoderReaderWrapper.cpp:177
(Diff revision 1)
>  }
>  
> +void
> +MediaDecoderReaderWrapper::UpdateDuration(const media::TimeUnit& aDuration)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());

the code isn't equivalent to using mirrors.

Tasks would have been dispatched using the tail dispatching mechanism, while now it's a task dispatch.

We've lost the state consistency that was provided by the mirror/canonical mechanism.

I would much prefer if you continued using mirror/canonical, it doesn't have to be in a AbstractMediaDecoder base class.
Comment on attachment 8884177 [details]
Bug 1378631. P1 - move the duration mirror from MediaDecoderReader to MediaDecoderReaderWrapper.

https://reviewboard.mozilla.org/r/155098/#review160168

> the code isn't equivalent to using mirrors.
> 
> Tasks would have been dispatched using the tail dispatching mechanism, while now it's a task dispatch.
> 
> We've lost the state consistency that was provided by the mirror/canonical mechanism.
> 
> I would much prefer if you continued using mirror/canonical, it doesn't have to be in a AbstractMediaDecoder base class.

I will move the duration mirror into MediaDecoderReaderWrapper since it is updated only by MDSM.
Comment on attachment 8884177 [details]
Bug 1378631. P1 - move the duration mirror from MediaDecoderReader to MediaDecoderReaderWrapper.

https://reviewboard.mozilla.org/r/155098/#review160906
Attachment #8884177 - Flags: review?(jyavenard) → review+
Comment on attachment 8884178 [details]
Bug 1378631. P2 - remove AbstractMediaDecoder::CanonicalDurationOrNull().

https://reviewboard.mozilla.org/r/155100/#review160908
Attachment #8884178 - Flags: review?(jyavenard) → review+
Comment on attachment 8884179 [details]
Bug 1378631. P3 - remove dead code.

https://reviewboard.mozilla.org/r/155102/#review160910

i would have merged this with P1... it adds no value to split this task.
Attachment #8884179 - Flags: review?(jyavenard) → review+
Attachment #8884179 - Attachment is obsolete: true
Thanks! P1 and P3 are merged.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d529f0b6575
P1 - move the duration mirror from MediaDecoderReader to MediaDecoderReaderWrapper. r=jya
https://hg.mozilla.org/integration/autoland/rev/d047630ee1ff
P2 - remove AbstractMediaDecoder::CanonicalDurationOrNull(). r=jya
https://hg.mozilla.org/mozilla-central/rev/8d529f0b6575
https://hg.mozilla.org/mozilla-central/rev/d047630ee1ff
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.