Don't call ResetDecode() in the destructor of MediaDecoderReader

RESOLVED FIXED in Firefox 44

Status

()

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

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
http://www.artima.com/cppsource/nevercall.html

ResetDecode() is a virtual method and it is bad to call it in the destructor. Furthermore, I don't see the necessity to call it in the destructor.

nsresult MediaDecoderReader::ResetDecode()
{
  VideoQueue().Reset(); <-- we don't care about it since it will be destroyed soon.
  AudioQueue().Reset(); <-- ditto.

  mAudioDiscontinuity = true; <-- don't care
  mVideoDiscontinuity = true; <-- don't care

  mBaseAudioPromise.RejectIfExists(CANCELED, __func__); <-- already rejected in Shutdown()
  mBaseVideoPromise.RejectIfExists(CANCELED, __func__); <-- ditto.

  return NS_OK;
}

We should be fine without calling ResetDecode() in the destructor.
(Assignee)

Updated

3 years ago
Assignee: nobody → jwwang
Blocks: 1213764
(Assignee)

Comment 1

3 years ago
Created attachment 8675489 [details]
MozReview Request: Bug 1215439 - Don't call ResetDecode() in the destructor of MediaDecoderReader. r=gerald.

Bug 1215439 - Don't call ResetDecode() in the destructor of MediaDecoderReader. r=gerald.
Attachment #8675489 - Flags: review?(gsquelart)
Comment on attachment 8675489 [details]
MozReview Request: Bug 1215439 - Don't call ResetDecode() in the destructor of MediaDecoderReader. r=gerald.

https://reviewboard.mozilla.org/r/22381/#review19975

Makes sense.
Attachment #8675489 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 3

3 years ago
Thanks for the review!
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/8aa35040b3bf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.