Closed Bug 1295073 Opened 4 years ago Closed 4 years ago

Remove the aRealTime parameter from the MediaDecoderStateMachine constructor

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file)

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: 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.
(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)
(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)
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... ;)
Yeah. I will submit a patch review in case I overlook something.
Attachment #8781838 - Flags: review?(cpearce)
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+
Thanks! Bug 1297301 is on the way.
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84090fe63313
Remove the aRealTime parameter from the MediaDecoderStateMachine constructor. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/84090fe63313
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.