Closed
Bug 1505121
Opened 6 years ago
Closed 6 years ago
Disable media decoders when recording/replaying
Categories
(Core Graveyard :: Web Replay, defect)
Core Graveyard
Web Replay
Tracking
(firefox65 fixed)
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
703 bytes,
patch
|
bryce
:
review+
|
Details | Diff | Splinter Review |
Bug 1304146 disables media elements when recording/replaying, but there are still times when media data is decoded in recording/replaying processes, which can lead to crashes. The attached patch disables these decoders when recording/replaying.
Attachment #9023056 -
Flags: review?(bvandyk)
Comment on attachment 9023056 [details] [diff] [review] patch Review of attachment 9023056 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nit and more explicit messaging. ::: dom/media/MediaFormatReader.cpp @@ +593,5 @@ > MediaResult result = MediaResult( > NS_ERROR_DOM_MEDIA_FATAL_ERR, > nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack))); > > + // Media playback is not supported when recording or replaying. See bug 1304146,. Nit: comma can be removed @@ +595,5 @@ > nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack))); > > + // Media playback is not supported when recording or replaying. See bug 1304146,. > + if (recordreplay::IsRecordingOrReplaying()) { > + return result; I think there's value to returning a more explicit result to make it clear we've hit this case. While I would think devs would generally know if they're in recording or replay, it doesn't hurt to be clear in case they're unaware of this impact or if this feature is mistakenly enabled. We could both log and update the error (the log could serve to replace above the comment if desired). Something like: `LOG("Media playback is not supported while recording or replaying -- see bug 1304146. Not creating decoder");` `return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, nsPrintfCString("error creating %s decoder: media playback is disabled while recording/replaying", TrackTypeToStr(aData.mTrack)));`
Attachment #9023056 -
Flags: review?(bvandyk) → review+
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/969b0caedb4d Disable media decoders when recording/replaying, r=bryce.
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Bryce Seager van Dyk (:bryce) from comment #1) > Comment on attachment 9023056 [details] [diff] [review] > patch > > Review of attachment 9023056 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with nit and more explicit messaging. > > ::: dom/media/MediaFormatReader.cpp > @@ +593,5 @@ > > MediaResult result = MediaResult( > > NS_ERROR_DOM_MEDIA_FATAL_ERR, > > nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack))); > > > > + // Media playback is not supported when recording or replaying. See bug 1304146,. > > Nit: comma can be removed > > @@ +595,5 @@ > > nsPrintfCString("error creating %s decoder", TrackTypeToStr(aData.mTrack))); > > > > + // Media playback is not supported when recording or replaying. See bug 1304146,. > > + if (recordreplay::IsRecordingOrReplaying()) { > > + return result; > > I think there's value to returning a more explicit result to make it clear > we've hit this case. While I would think devs would generally know if > they're in recording or replay, it doesn't hurt to be clear in case they're > unaware of this impact or if this feature is mistakenly enabled. > > We could both log and update the error (the log could serve to replace above > the comment if desired). Something like: > `LOG("Media playback is not supported while recording or replaying -- see > bug 1304146. Not creating decoder");` > `return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, nsPrintfCString("error > creating %s decoder: media playback is disabled while recording/replaying", > TrackTypeToStr(aData.mTrack)));` Thanks, I included the more detailed MediaResult, but calling LOG() here doesn't work as the macro doesn't compile when called in this function (LOG's expansion uses 'this' on a templated function that doesn't instantiate for MediaFormatReader::DecoderFactory).
Ah, makes sense. I'd overlooked it is a this flavour of log macro for MediaFormatReader.
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/969b0caedb4d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Updated•4 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•