Closed Bug 1228939 Opened 4 years ago Closed 4 years ago

Remove MediaDecoderReader::IsMediaSeekable()

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(2 files)

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: nobody → jwwang
Priority: -- → P2
Bug 1228939. Part 1 - use MediaEventSource to notify the decoder when the media is not seekable. r=jya.
Attachment #8694095 - Flags: review?(jyavenard)
Bug 1228939. Part 2 - remove unused code. r=jya.
Attachment #8694097 - Flags: review?(jyavenard)
Attachment #8694095 - Flags: review?(jyavenard)
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)
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/
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+
(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.
(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.
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> merge both patches into 1.
will do.
https://hg.mozilla.org/mozilla-central/rev/19c212559258
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.