Closed Bug 934106 Opened 7 years ago Closed 7 years ago

Reduce ImageClient holding video frame number to one

Categories

(Core :: Graphics: Layers, defect)

26 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
Blocks: 933376
No longer depends on: 933376
blocking-b2g: --- → koi?
Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

nical, how do you think about the change?
Attachment #826301 - Flags: feedback?(nical.bugzilla)
Assignee: nobody → sotaro.ikeda.g
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.
Attachment #826301 - Flags: feedback?(nical.bugzilla) → feedback+
Comment on attachment 826301 [details] [diff] [review]
patch - Reduce ImageClient holding video frame number to one

Proceed to review.
Attachment #826301 - Flags: review?(nical.bugzilla)
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.
Flags: needinfo?(sotaro.ikeda.g)
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.
Flags: needinfo?(sotaro.ikeda.g)
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.
Attachment #826301 - Flags: review?(nical.bugzilla) → review+
"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.
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.
Ok design regression in that case :)
(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=579dcb17c079

tryserver result is good.
blocking-b2g: koi? → koi+
Comittable patch. Single buffer is used only when new texture is enabled. Carry 'r=nical+'.
Comment on attachment 827562 [details] [diff] [review]
patch v2 - Reduce ImageClient holding video frame number to one

Carry 'r+=nical'.
Attachment #827562 - Flags: review+
Attachment #826301 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 934870
No longer blocks: 934870
Duplicate of this bug: 934870
https://hg.mozilla.org/mozilla-central/rev/0a1c235131ca
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 944132
No longer blocks: 944132
Blocks: 944167
No longer blocks: 944167
You need to log in before you can comment on or make changes to this bug.