Remove the aRealTime parameter from the MediaDecoderStateMachine constructor

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
https://hg.mozilla.org/mozilla-central/file/tip/dom/media/raw/RawDecoder.cpp#l13

Only RawDecoder will set this parameter to true.

The raw format should be different only in the way it is decoded. It should be no different in how it is played.

Hi Chris,
Is there any reason for RawDecoder to set this parameter to true?
(Assignee)

Updated

2 years ago
Assignee: nobody → jwwang
Flags: needinfo?(cpearce)
Priority: -- → P3
RawDecoder is used on Android, I think for something related to camera input? So the most recent frame is desired at all times for RawReader. I don't think we can remove it, unless we can somehow get the same behaviour some other way.
Flags: needinfo?(cpearce)
That is to say, the most recent frame is desired from RawReader, because it's for streaming camera input. So we want the lowest latency possible.
(Assignee)

Comment 3

2 years ago
(In reply to Chris Pearce (:cpearce) from comment #2)
> That is to say, the most recent frame is desired from RawReader, because
> it's for streaming camera input. So we want the lowest latency possible.

Our compositor decides when to render video frames based on their timestamps. The most recent frame won't work as expected as before because its timestamp must be correct to be rendered at the right time. There is no exception for the video frames produced by RawReader.
Flags: needinfo?(cpearce)
(Assignee)

Comment 4

2 years ago
(In reply to Chris Pearce (:cpearce) from comment #1)
> RawDecoder is used on Android, I think for something related to camera
> input?

IIRC, getUserMedia uses MediaStream without MediaDecoder or MDSM involved.
(In reply to JW Wang [:jwwang] from comment #3)
> (In reply to Chris Pearce (:cpearce) from comment #2)
> > That is to say, the most recent frame is desired from RawReader, because
> > it's for streaming camera input. So we want the lowest latency possible.
> 
> Our compositor decides when to render video frames based on their
> timestamps. The most recent frame won't work as expected as before because
> its timestamp must be correct to be rendered at the right time. There is no
> exception for the video frames produced by RawReader.

If needed, we could have the VideoSink clear everything but the latest frame.

(In reply to JW Wang [:jwwang] from comment #4)
> (In reply to Chris Pearce (:cpearce) from comment #1)
> > RawDecoder is used on Android, I think for something related to camera
> > input?
> 
> IIRC, getUserMedia uses MediaStream without MediaDecoder or MDSM involved.

I don't understand well how that all connects, so I'd defer to you on that.

I asked an intern to try removing the RawReader, and the Android build broke. I'd be happy if we could figure out what is using the RawReader, and see if we can figure out a way to make it use something else. So we could remove the RawReader, and this parameter.
Flags: needinfo?(cpearce)
(Assignee)

Comment 6

2 years ago
It is interesting that there is no code in MDSM and VideoSink concerning about "render the most recent frame as soon as possible". So I think it will still work fine to remove the aRealTime parameter but keep RawDecoder/RawReader in order not to break Android builds.
You mean, no one has noticed that we broke it, so it's unlikely they'll notice we break it further... ;)
(Assignee)

Comment 8

2 years ago
Yeah. I will submit a patch review in case I overlook something.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8781838 - Flags: review?(cpearce)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8781838 [details]
Bug 1295073 - Remove the aRealTime parameter from the MediaDecoderStateMachine constructor.

https://reviewboard.mozilla.org/r/72162/#review71556

Great! Can you remove the RawDecoder as well, or find someone in Taipei to do it?
Attachment #8781838 - Flags: review?(cpearce) → review+
(Assignee)

Comment 11

2 years ago
Thanks! Bug 1297301 is on the way.
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84090fe63313
Remove the aRealTime parameter from the MediaDecoderStateMachine constructor. r=cpearce

Comment 14

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