Open Bug 1293963 (Phase_1_Shutdown_Decoder) Opened 4 years ago Updated 3 years ago

[Meta] Suspend-video-decoder: phase-1 shipping

Categories

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

defect

Tracking

()

Tracking Status
firefox51 --- affected

People

(Reporter: kaku, Assigned: kaku)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

Per offline discussion, we are going to ship the phase 1 of suspend-video-decoder. In the phase 1, we target on the SAFE cases. Our assumption for SAFE cases is that for videos under certain resolution, the resuming time could be ignored.
Blocks: 1276556
So, the question is "What exact is the target resolution? and is the target resolution platform-dependent?" 

Looks like that we don't have the telemetry for "resuming time v.s. resolution", we might this data to help deciding the target resolution.
In discussions, we've said to limit to "480" video or less. I took that to mean the default resolution of YouTube videos. The video ad I'm testing with is 720 × 406.
Yes, we did and I will implement it with 480P for now. But at the same time, we can still collect telemetry data for more rigorous decision later. Thanks :gerald for implementing the telemetry.
Depends on: 1294349
Since bug 1294656 has been landed, we can also enable the suspend-video-decoder feature for those video files without audio track, which could be resumed quickly by fast seek.
Depends on: 1294656
Summary: Only enable background-video-decoder-suspend for videos under certain resolution → Suspend-video-decoder: phase-1 shipping
Assignee: nobody → kaku
Depends on: 1295921, 1284389
Comment on attachment 8788036 [details]
Bug 1293963 - phase-1 shipping;

https://reviewboard.mozilla.org/r/76580/#review74716

1. Extract all the policy code to a function so it is easier to maintain when we add more policies.
2. Do we need to switch the "shipping phase" back and forth? If not, there is no need to encode the concept in the code.
3. IIUC, VideoInfo::mDisplay is a scaled size and you should look at mImage. Please ask come gfx guys to confirm it.
Attachment #8788036 - Flags: review?(jwwang) → review-
Comment on attachment 8788036 [details]
Bug 1293963 - phase-1 shipping;

https://reviewboard.mozilla.org/r/76580/#review74716

> 2. Do we need to switch the "shipping phase" back and forth? If not, there is no need to encode the concept in the code.

I think that we could turn the nightly channel into phase-0 again once the phase-1 code has been checked into aurora, so that we can collect more telemetry data. 
Alos, keeps this concept might help wrting test cases.
Comment on attachment 8788036 [details]
Bug 1293963 - phase-1 shipping;

https://reviewboard.mozilla.org/r/76580/#review74716

It looks like a maintainace hazard to have different phases on difference release channels. It is also confusing when you have to check the phase in order to write a test case to run correctly on that phase.
Comment on attachment 8788036 [details]
Bug 1293963 - phase-1 shipping;

https://reviewboard.mozilla.org/r/76580/#review74732
Attachment #8788036 - Flags: review?(jwwang) → review+
Thanks for the review!

Try looks good at https://treeherder.mozilla.org/#/jobs?repo=try&revision=51d36fcddaf2
I think we should NOT land this patch before bug 1284389 and bug 1295921 being resolved, right?
Flags: needinfo?(dglastonbury)
(In reply to Tzuhao Kuo [:kaku] from comment #13)
> I think we should NOT land this patch before bug 1284389 and bug 1295921
> being resolved, right?

Yeah, let's hang off until we can land bug 1295921. I'm working hard of getting head way into that bug.
Flags: needinfo?(dglastonbury)
Thanks for taking the tough bug!

I will start a long PTO since next Wed. on (around two weeks, until 10/2), so once you finished bug 1295921, please feel free to take this one and land the patch if you want.
Depends on: 1305338
See Also: → 1308184
Summary: Suspend-video-decoder: phase-1 shipping → [Meta] Suspend-video-decoder: phase-1 shipping
Blocks: 1308184
Depends on: 1309492
Depends on: 1309494
Alias: Phase_1_Shutdown_Decoder
Depends on: 1346116
Depends on: 1346120
Depends on: 1346235
Depends on: 1349461
Depends on: 1348864
Depends on: 1349456, 1345403, 1349459
No longer depends on: 1284389, 1295921, 1349461
No longer blocks: 1276556
Depends on: 1276556
QA Contact: bogdan.surd
See Also: → 1355407
See Also: 1355407
No longer depends on: 1348864
Depends on: 1348864
Depends on: 1358412
Depends on: 1365581
No longer blocks: 1365584
Depends on: 1365584
No longer depends on: 1358412
Depends on: 1369970
Attachment #8788036 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.