Remove MediaDecoderReader::IsMediaSeekable()

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
We will use MediaEventSource to notify the decoder that this media is not seekable. This eliminate the need for cross-thread reading from members of the readers and make a more consistent thread-model for the readers.
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Priority: -- → P2
(Assignee)

Comment 2

2 years ago
Created attachment 8694095 [details]
MozReview Request: Bug 1228939. Part 1 - 1. add mSeekable to MediaInfo. 2. use MediaEventSource to notify the decoder when the media is not seekable. r=jya.

Bug 1228939. Part 1 - use MediaEventSource to notify the decoder when the media is not seekable. r=jya.
Attachment #8694095 - Flags: review?(jyavenard)
(Assignee)

Comment 3

2 years ago
Created attachment 8694097 [details]
MozReview Request: Bug 1228939. Part 2 - remove unused code. r=jya.

Bug 1228939. Part 2 - remove unused code. r=jya.
Attachment #8694097 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Attachment #8694095 - Flags: review?(jyavenard)
(Assignee)

Comment 5

2 years ago
Comment on attachment 8694095 [details]
MozReview Request: Bug 1228939. Part 1 - 1. add mSeekable to MediaInfo. 2. use MediaEventSource to notify the decoder when the media is not seekable. r=jya.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26657/diff/1-2/
Attachment #8694095 - Attachment description: MozReview Request: Bug 1228939. Part 1 - use MediaEventSource to notify the decoder when the media is not seekable. r=jya. → MozReview Request: Bug 1228939. Part 1 - 1. add mSeekable to MediaInfo. 2. use MediaEventSource to notify the decoder when the media is not seekable. r=jya.
Attachment #8694095 - Flags: review?(jyavenard)
(Assignee)

Comment 6

2 years ago
Comment on attachment 8694097 [details]
MozReview Request: Bug 1228939. Part 2 - remove unused code. r=jya.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26659/diff/1-2/
(Assignee)

Comment 7

2 years ago
Per discussion, add mMediaSeekable to MediaInfo to indicate if this media is seekable or not when reading metadata.
Comment on attachment 8694095 [details]
MozReview Request: Bug 1228939. Part 1 - 1. add mSeekable to MediaInfo. 2. use MediaEventSource to notify the decoder when the media is not seekable. r=jya.

https://reviewboard.mozilla.org/r/26657/#review24241

::: dom/media/MediaInfo.h:404
(Diff revision 2)
> +  bool mMediaSeekable = true;

personally I like this being done in the constructor. not a fan of C++11 inline member initialisation
Attachment #8694095 - Flags: review?(jyavenard) → review+
Comment on attachment 8694097 [details]
MozReview Request: Bug 1228939. Part 2 - remove unused code. r=jya.

https://reviewboard.mozilla.org/r/26659/#review24243

I would have merged part 1 and part2 as part 1 includes piece of code that is removed from part2 and you get to wonder where the logic come from. would be more apparent in a combined patch
Attachment #8694097 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 10

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Comment on attachment 8694095 [details]
> MozReview Request: Bug 1228939. Part 1 - 1. add mSeekable to MediaInfo. 2.
> use MediaEventSource to notify the decoder when the media is not seekable.
> r=jya.
> 
> https://reviewboard.mozilla.org/r/26657/#review24241
> 
> ::: dom/media/MediaInfo.h:404
> (Diff revision 2)
> > +  bool mMediaSeekable = true;
> 
> personally I like this being done in the constructor. not a fan of C++11
> inline member initialisation

It comes in handy when every constructor initializes the member with the same value or when there is no constructor at all which is the case of MediaInfo.
(Assignee)

Comment 11

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Comment on attachment 8694097 [details]
> MozReview Request: Bug 1228939. Part 2 - remove unused code. r=jya.
> 
> https://reviewboard.mozilla.org/r/26659/#review24243
> 
> I would have merged part 1 and part2 as part 1 includes piece of code that
> is removed from part2 and you get to wonder where the logic come from. would
> be more apparent in a combined patch

Which portion of part2 do you mean that can be moved to part1? part2 is all about removing code.
(In reply to JW Wang [:jwwang] from comment #10)

> It comes in handy when every constructor initializes the member with the
> same value or when there is no constructor at all which is the case of
> MediaInfo.

You can use delegated constructor now https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code
so you don't have to initialize the value in each constructor. only one
(In reply to JW Wang [:jwwang] from comment #11)

> Which portion of part2 do you mean that can be moved to part1? part2 is all
> about removing code.

merge both patches into 1.
(Assignee)

Comment 14

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> merge both patches into 1.
will do.

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19c212559258
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.