934106 - Reduce ImageClient holding video frame number to one

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #933376 +++

In v1.1, Compositor(b2g process) side hold one video frame. But since v1.2, Compositor side hold 2 video frames. There seems no necessity to hold 2 video frames on Compositor side.

Increase the number of buffers held by Compositor side is same to decrease the number of buffers used by hw codec/camera hw side.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce ImageClient holding video frame number to one

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

nical, how do you think about the change?

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

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

Double buffered ImageClient was around mostly because of a quirk of the deprecated textures, now it should work with single buffering.

Comment 4

[Sotaro Ikeda [:sotaro]]

Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

Proceed to review.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Sotaro, is this a must for 1.2?  What are the benefits?  I recall us running into a few problems switching from and to single/double buffering, I'd rather avoid it here.

Comment 6

[Sotaro Ikeda [:sotaro]]

hw codec's video handling is very very tight. Buffer shortage could cause the problem like Bug 863441. And this bug might be causing the Bug 933376. But it is not clear yet.

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

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

R+, but carefully test that this does not break anything, including on OSX. At least a full green try run would make me feel comfortable.
Switching to single buffering would make us use less memory (the equivalent of one frame) during video playback, which is really nice on B2G.
We needed double buffering because of how deprecated textures worked, and one of the breakage I caused related to buffering was to switch to single buffering before switching to the new textures. Not sure if I tried again later and failed for some other reason after we enabled new textures.

Comment 8

[Andreas Gal :gal]

"In v1.1, Compositor(b2g process) side hold one video frame. But since v1.2, Compositor side hold 2 video frames. There seems no necessity to hold 2 video frames on Compositor side." Several bugs are on file with playback problems we didn't have in 1.1.

Sounds like a pretty clear regression from 1.1 to me.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

It was a design decision, using double buffering, instead of single buffering.  We should treat this change as a one to design, and make sure we understand all the implications.

Comment 10

[Andreas Gal :gal]

Ok design regression in that case :)

Comment 11

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=579dcb17c079

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=579dcb17c079

tryserver result is good.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Reduce ImageClient holding video frame number to one

Comittable patch. Single buffer is used only when new texture is enabled. Carry 'r=nical+'.

Comment 14

[Sotaro Ikeda [:sotaro]]

Comment on attachment 827562 [details] [diff] [review]
patch v2 - Reduce ImageClient holding video frame number to one

Carry 'r+=nical'.

Comment 15

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=86f2e2cb810b
Just reconfirm the result.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0a1c235131ca

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e4d673ad84b0

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a1c235131ca