Closed Bug 1121841 Opened 5 years ago Closed 5 years ago

InvokeAndRetry can falsely report EOS

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(1 file)

Consider the following situation in InvokeAndRetry:

(1) We invoke the demuxer, it does a read.
(2) The read fails, and we record that on the MP4Stream.
(3) InvokeAndRetry notices the failed read, and retries as a blocking read.
(4) InvokeAndRetry calls back into the demuxer, which errors out for some unrelated reason.
(5) InvokeAndRetry inspects the MP4Stream again, sees the same failed read, and concludes the read failed twice.

This doesn't affect correctness, since we'll throw an error in either case. But it's confusing, because the NS_WARNINGs will describe the incorrect error condition.

Fixing this is trivial. I think we should also at least uplift it to aurora, because it's a non-risk and helpful for debugging.
Attachment #8549412 - Flags: review?(jyavenard)
Comment on attachment 8549412 [details] [diff] [review]
Clear the failed read after checking it. v1

Review of attachment 8549412 [details] [diff] [review]:
-----------------------------------------------------------------

I've would have done so in LastReadFailed instead, so no need for public function
Attachment #8549412 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8549412 [details] [diff] [review]
> Clear the failed read after checking it. v1
> 
> Review of attachment 8549412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've would have done so in LastReadFailed instead, so no need for public
> function

Yeah, but then the API has a non-obvious side-effect.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3abade3e4eb4

This is basically no risk, so we might as well uplift it to make debugging easier.
Flags: needinfo?(giles)
Comment on attachment 8549412 [details] [diff] [review]
Clear the failed read after checking it. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]:
[Describe test coverage new/current, TBPL]: presuming green on inbound.
[Risks and why]: Low. Affects mp4 playback but small and self-contained cleanup in error handling.
[String/UUID change made/needed]: None.
Flags: needinfo?(giles)
Attachment #8549412 - Flags: approval-mozilla-beta?
Attachment #8549412 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3abade3e4eb4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8549412 - Flags: approval-mozilla-beta?
Attachment #8549412 - Flags: approval-mozilla-beta+
Attachment #8549412 - Flags: approval-mozilla-aurora?
Attachment #8549412 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.