Closed Bug 1390748 Opened 6 years ago Closed 6 years ago

Most ChannelMediaDecoder subclasses don't really need to be subclasses

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla57

Tracking Flags:

Tracking

Status

firefox57

---

fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(10 files)

Bug 1390748 - Remove OggDecoder::CreateStateMachine(). 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Make MediaDecoder::CreateStateMachine() non-virtual. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Remove unnececssary 'public:' in MediaDecoder.h. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Create default MediaDecoder::GetMozDebugReaderData() implementation. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Pre-declare MediaDecoder in HTMLMediaElement. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Create DecoderTraits::IsSupportedType(const MediaContainerType&). 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Remove some unnecessary includes. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Remove unnecessary includes in *Decoders. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details
Bug 1390748 - Remove usage of mozilla::Vector from mp4_demuxer::ByteReader. 6 years ago Chris Pearce [:cpearce (Not reading bugmail)] 59 bytes, text/x-review-board-request	jwwang : review+	Details

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Description

•

6 years ago

Most of the subclasses of ChannelMediaDecoder don't actually need to be subclasses; they typically are subclasses so they can have a custom CreateStateMachine() or a GetMozDebugReaderData(). They also have a CloneImpl() which knows the true type of the instance, but we don't need that if we don't have subclasses.

I think we can remove most subclasses of ChannelMediaDecoder.

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/169004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169004/

Comment hidden (mozreview-request)

MediaDecoder::CreateStateMachine is only virtual so that Ogg can attach
the reader's metadata/seekable produces to its chaining event.

The MediaSourceDecoder also overrides CreateStateMachine(), but it's not
called by anything external, so its implementation doesn't actually need
to be virtual.

Review commit: https://reviewboard.mozilla.org/r/169006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169006/

Comment hidden (mozreview-request)

There's an existing 'public:' further up.

Review commit: https://reviewboard.mozilla.org/r/169008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169008/

Comment hidden (mozreview-request)

We have three implementations, in the MP4, WebM and MediaSource decoders.  The
WebM and MP4 are the same. Ogg and other decoders don't have an implementation,
but if we create a default implementation in MediaDecoder, they'll get it for
free. MediaSourceDecoder needs a custom override still.

Review commit: https://reviewboard.mozilla.org/r/169010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169010/

Comment hidden (mozreview-request)

I noticed that touching MediaDecoder rebuilds a lot of seemingly unrelated
code. This is because HTMLMediaElement includes MediaDecoder.h, and
HTMLMediaElement is included in a number of places. Having HTMLMediaElement.h
predeclare rather than include fixes it.

Review commit: https://reviewboard.mozilla.org/r/169012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169012/

Comment hidden (mozreview-request)

Most ChannelMediaDecoder::CloneImpl() functions just check to see whether
their "is enabled" pref is still true, and then clone their true type.

If we had a function to check whether the decoder for an arbitrary type
was still enabled, we'd not need the "is enabled" checks in the CloneImpl()
implementations. We'd then have removed the last custom behaviour in the
ChannelMediaDecoder subclasses.

Review commit: https://reviewboard.mozilla.org/r/169014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169014/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/169016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169016/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/169018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169018/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/169020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169020/

Comment hidden (mozreview-request)

It appears to be unused.

Review commit: https://reviewboard.mozilla.org/r/169022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169022/

JW Wang [:jwwang] [:jw_wang]

Comment 11

•

6 years ago

mozreview-review

Comment on attachment 8897713 [details]
Bug 1390748 - Remove OggDecoder::CreateStateMachine().

https://reviewboard.mozilla.org/r/169004/#review174342

Attachment #8897713 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 12

•

6 years ago

mozreview-review

Comment on attachment 8897714 [details]
Bug 1390748 - Make MediaDecoder::CreateStateMachine() non-virtual.

https://reviewboard.mozilla.org/r/169006/#review174344

Attachment #8897714 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 13

•

6 years ago

mozreview-review

Comment on attachment 8897715 [details]
Bug 1390748 - Remove unnececssary 'public:' in MediaDecoder.h.

https://reviewboard.mozilla.org/r/169008/#review174346

Attachment #8897715 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 14

•

6 years ago

mozreview-review

Comment on attachment 8897716 [details]
Bug 1390748 - Create default MediaDecoder::GetMozDebugReaderData() implementation.

https://reviewboard.mozilla.org/r/169010/#review174348

Attachment #8897716 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 15

•

6 years ago

mozreview-review

Comment on attachment 8897717 [details]
Bug 1390748 - Pre-declare MediaDecoder in HTMLMediaElement.

https://reviewboard.mozilla.org/r/169012/#review174350

Attachment #8897717 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 16

•

6 years ago

mozreview-review

Comment on attachment 8897718 [details]
Bug 1390748 - Create DecoderTraits::IsSupportedType(const MediaContainerType&).

https://reviewboard.mozilla.org/r/169014/#review174352

Attachment #8897718 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 17

•

6 years ago

mozreview-review

Comment on attachment 8897719 [details]
Bug 1390748 - Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder.

https://reviewboard.mozilla.org/r/169016/#review174354

Attachment #8897719 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 18

•

6 years ago

mozreview-review

Comment on attachment 8897720 [details]
Bug 1390748 - Remove some unnecessary includes.

https://reviewboard.mozilla.org/r/169018/#review174356

Attachment #8897720 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 19

•

6 years ago

mozreview-review

Comment on attachment 8897721 [details]
Bug 1390748 - Remove unnecessary includes in *Decoders.

https://reviewboard.mozilla.org/r/169020/#review174358

Attachment #8897721 - Flags: review?(jwwang) → review+

JW Wang [:jwwang] [:jw_wang]

Comment 20

•

6 years ago

mozreview-review

Comment on attachment 8897722 [details]
Bug 1390748 - Remove usage of mozilla::Vector from mp4_demuxer::ByteReader.

https://reviewboard.mozilla.org/r/169022/#review174360

Nice cleanup!

Attachment #8897722 - Flags: review?(jwwang) → review+

Comment hidden (mozreview-request)

Comment on attachment 8897713 [details]
Bug 1390748 - Remove OggDecoder::CreateStateMachine().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169004/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897714 [details]
Bug 1390748 - Make MediaDecoder::CreateStateMachine() non-virtual.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169006/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897715 [details]
Bug 1390748 - Remove unnececssary 'public:' in MediaDecoder.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169008/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897716 [details]
Bug 1390748 - Create default MediaDecoder::GetMozDebugReaderData() implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169010/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897717 [details]
Bug 1390748 - Pre-declare MediaDecoder in HTMLMediaElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169012/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897718 [details]
Bug 1390748 - Create DecoderTraits::IsSupportedType(const MediaContainerType&).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169014/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897719 [details]
Bug 1390748 - Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169016/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897720 [details]
Bug 1390748 - Remove some unnecessary includes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169018/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897721 [details]
Bug 1390748 - Remove unnecessary includes in *Decoders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169020/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8897722 [details]
Bug 1390748 - Remove usage of mozilla::Vector from mp4_demuxer::ByteReader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169022/diff/1-2/

Pulsebot

Comment 31

•

6 years ago

Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/634fe076d134
Remove OggDecoder::CreateStateMachine(). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5d3b637d2485
Make MediaDecoder::CreateStateMachine() non-virtual. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5fa77990472e
Remove unnececssary 'public:' in MediaDecoder.h. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/bf78a8dce9c5
Create default MediaDecoder::GetMozDebugReaderData() implementation. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/0e77809e7bc2
Pre-declare MediaDecoder in HTMLMediaElement. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/7dadac869038
Create DecoderTraits::IsSupportedType(const MediaContainerType&). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/14d421ace6cc
Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fc8989be30f3
Remove some unnecessary includes. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/84e8bf01c856
Remove unnecessary includes in *Decoders. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/02b36ee5fb17
Remove usage of mozilla::Vector from mp4_demuxer::ByteReader. r=jwwang

Phil Ringnalda (:philor)

Comment 32

•

6 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/e61998d0ccba for https://treeherder.mozilla.org/logviewer.html#?job_id=123774888&repo=autoland

Comment hidden (mozreview-request)

Comment on attachment 8897713 [details]
Bug 1390748 - Remove OggDecoder::CreateStateMachine().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169004/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897714 [details]
Bug 1390748 - Make MediaDecoder::CreateStateMachine() non-virtual.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169006/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897715 [details]
Bug 1390748 - Remove unnececssary 'public:' in MediaDecoder.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169008/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897716 [details]
Bug 1390748 - Create default MediaDecoder::GetMozDebugReaderData() implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169010/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897717 [details]
Bug 1390748 - Pre-declare MediaDecoder in HTMLMediaElement.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169012/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897718 [details]
Bug 1390748 - Create DecoderTraits::IsSupportedType(const MediaContainerType&).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169014/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897719 [details]
Bug 1390748 - Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169016/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897720 [details]
Bug 1390748 - Remove some unnecessary includes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169018/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897721 [details]
Bug 1390748 - Remove unnecessary includes in *Decoders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169020/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8897722 [details]
Bug 1390748 - Remove usage of mozilla::Vector from mp4_demuxer::ByteReader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169022/diff/2-3/

Pulsebot

Comment 43

•

6 years ago

Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57be2e971ae2
Remove OggDecoder::CreateStateMachine(). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/b86afd45a710
Make MediaDecoder::CreateStateMachine() non-virtual. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ad5c7eb1bfe1
Remove unnececssary 'public:' in MediaDecoder.h. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2964dba33ef8
Create default MediaDecoder::GetMozDebugReaderData() implementation. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/4a31e4302e03
Pre-declare MediaDecoder in HTMLMediaElement. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/94c0ec4bf19e
Create DecoderTraits::IsSupportedType(const MediaContainerType&). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d3c000525403
Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/9172b8d6ce3e
Remove some unnecessary includes. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3f9350bf23ff
Remove unnecessary includes in *Decoders. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2674b8eaba32
Remove usage of mozilla::Vector from mp4_demuxer::ByteReader. r=jwwang

Carsten Book [:Tomcat]

Comment 44

•

6 years ago

had to back this out for breaking android like https://treeherder.mozilla.org/logviewer.html#?job_id=123788151&repo=autoland

Flags: needinfo?(cpearce)

Pulsebot

Comment 45

•

6 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87e969d3da2b
Backed out changeset 2674b8eaba32 
https://hg.mozilla.org/integration/autoland/rev/1f08c4e92e4a
Backed out changeset 3f9350bf23ff 
https://hg.mozilla.org/integration/autoland/rev/13db236c2883
Backed out changeset 9172b8d6ce3e 
https://hg.mozilla.org/integration/autoland/rev/17e31f111ffd
Backed out changeset d3c000525403 
https://hg.mozilla.org/integration/autoland/rev/d26588dbac29
Backed out changeset 94c0ec4bf19e 
https://hg.mozilla.org/integration/autoland/rev/9e7c53bf39af
Backed out changeset 4a31e4302e03 
https://hg.mozilla.org/integration/autoland/rev/d9ba21e01f8d
Backed out changeset 2964dba33ef8 
https://hg.mozilla.org/integration/autoland/rev/71f49d8615be
Backed out changeset ad5c7eb1bfe1 
https://hg.mozilla.org/integration/autoland/rev/a1209f7f7585
Backed out changeset b86afd45a710 
https://hg.mozilla.org/integration/autoland/rev/eb39976c4a0f
Backed out changeset 57be2e971ae2 for bustage

Comment hidden (mozreview-request)

Comment on attachment 8897719 [details]
Bug 1390748 - Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169016/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8897720 [details]
Bug 1390748 - Remove some unnecessary includes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169018/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8897721 [details]
Bug 1390748 - Remove unnecessary includes in *Decoders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169020/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8897722 [details]
Bug 1390748 - Remove usage of mozilla::Vector from mp4_demuxer::ByteReader.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169022/diff/3-4/

Pulsebot

Comment 50

•

6 years ago

Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04677e50eae4
Remove OggDecoder::CreateStateMachine(). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a297b34e8d08
Make MediaDecoder::CreateStateMachine() non-virtual. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5ece0caad34c
Remove unnececssary 'public:' in MediaDecoder.h. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/047db39bc8b4
Create default MediaDecoder::GetMozDebugReaderData() implementation. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/1377dc708aa9
Pre-declare MediaDecoder in HTMLMediaElement. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f19970af5f01
Create DecoderTraits::IsSupportedType(const MediaContainerType&). r=jwwang
https://hg.mozilla.org/integration/autoland/rev/557168f0da99
Remove ChannelMediaDecoder::CloneImpl() and remove subclasses of ChannelMediaDecoder. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/43b00dfe7c29
Remove some unnecessary includes. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/cfe1b2eb0610
Remove unnecessary includes in *Decoders. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/859e657f26dc
Remove usage of mozilla::Vector from mp4_demuxer::ByteReader. r=jwwang

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

Comment 51

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/04677e50eae4
https://hg.mozilla.org/mozilla-central/rev/a297b34e8d08
https://hg.mozilla.org/mozilla-central/rev/5ece0caad34c
https://hg.mozilla.org/mozilla-central/rev/047db39bc8b4
https://hg.mozilla.org/mozilla-central/rev/1377dc708aa9
https://hg.mozilla.org/mozilla-central/rev/f19970af5f01
https://hg.mozilla.org/mozilla-central/rev/557168f0da99
https://hg.mozilla.org/mozilla-central/rev/43b00dfe7c29
https://hg.mozilla.org/mozilla-central/rev/cfe1b2eb0610
https://hg.mozilla.org/mozilla-central/rev/859e657f26dc

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox57: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

Chris Pearce [:cpearce (Not reading bugmail)]

Assignee

Updated

•

6 years ago

Flags: needinfo?(cpearce)

Sylvestre Ledru [:Sylvestre]

Updated

•

5 years ago

Assignee: nobody → cpearce

You need to log in before you can comment on or make changes to this bug.