1295073 - Remove the aRealTime parameter from the MediaDecoderStateMachine constructor

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

You mean, no one has noticed that we broke it, so it's unlikely they'll notice we break it further... ;)

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Yeah. I will submit a patch review in case I overlook something.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1295073 - Remove the aRealTime parameter from the MediaDecoderStateMachine constructor.

Review commit: https://reviewboard.mozilla.org/r/72162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72162/

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Thanks! Bug 1297301 is on the way.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8781838 [details]
Bug 1295073 - Remove the aRealTime parameter from the MediaDecoderStateMachine constructor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72162/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84090fe63313
Remove the aRealTime parameter from the MediaDecoderStateMachine constructor. r=cpearce

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84090fe63313