Closed Bug 1345403 Opened 3 years ago Closed 3 years ago

Mark a video element as taint and never suspend it again if it is passed into drawImage()

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kaku, Assigned: kaku)

References

(Blocks 3 open bugs)

Details

Attachments

(5 files)

Spawn from bug 1295921 comment 208.
Assignee: nobody → kaku
Blocks: 1295921
Status: NEW → ASSIGNED
This bug is used to land patches 1~6 of Bug 129592. However, I have applied some modifications, and the original patches are old, so will ask for a review again!
(In reply to Tzuhao Kuo [:kaku] from comment #1)
> This bug is used to land patches 1~6 of Bug 129592. However, I have applied
> some modifications, and the original patches are old, so will ask for a
> review again!

Sorry, it should be Bug 1295921, not Bug 129592.
Comment on attachment 8845217 [details]
Bug 1345403 part 1 - Track decoder tainting;

https://reviewboard.mozilla.org/r/118408/#review120338
Attachment #8845217 - Flags: review?(jwwang) → review+
Comment on attachment 8845218 [details]
Bug 1345403 part 2 - Mark element tainted when DrawImage is used;

https://reviewboard.mozilla.org/r/118410/#review120344

::: dom/html/HTMLMediaElement.cpp:4761
(Diff revision 1)
>    }
>  
>    AddMediaElementToURITable();
>  
> +  // Notify the decoder of suspend taint.
> +  mDecoder->SetSuspendTaint(mHasSuspendTaint);

Move this just below |mDecoder->SetPlaybackRate(mPlaybackRate)| to group code setting MediaDecoder attributes.
Attachment #8845218 - Flags: review?(jwwang) → review+
Comment on attachment 8845219 [details]
Bug 1345403 part 3 - Test element becomes tainted by DrawImage;

https://reviewboard.mozilla.org/r/118412/#review120346
Attachment #8845219 - Flags: review?(jwwang) → review+
Comment on attachment 8845219 [details]
Bug 1345403 part 3 - Test element becomes tainted by DrawImage;

https://reviewboard.mozilla.org/r/118412/#review120348

::: dom/webidl/HTMLMediaElement.webidl:217
(Diff revision 1)
>    [Throws, Pref="media.seekToNextFrame.enabled"]
>    Promise<void> seekToNextFrame();
>  };
>  
>  /*
> - * This is an API for simulating visibility changes to help debug and write
> + * This APIs are used to help debug and write tests about suspend-video-decoding.

Use singular form because this 'is' an API which has multiple functions.
Comment on attachment 8845220 [details]
Bug 1345403 part 4 - Clean up suspend timer canceling;

https://reviewboard.mozilla.org/r/118414/#review120352

::: dom/media/MediaDecoderStateMachine.h:126
(Diff revision 1)
>    PlaybackEnded,
>    SeekStarted,
>    Invalidate,
>    EnterVideoSuspend,
> -  ExitVideoSuspend
> +  ExitVideoSuspend,
> +  BeginVideoSuspend,

The names are confusing. How about "StartVideoSuspendTimer" and "CancelVideoSuspendTimer"?
Attachment #8845220 - Flags: review?(jwwang) → review+
Comment on attachment 8845221 [details]
Bug 1345403 part 5 - Test video suspend canceling;

https://reviewboard.mozilla.org/r/118416/#review120356
Attachment #8845221 - Flags: review?(jwwang) → review+
Comment on attachment 8845219 [details]
Bug 1345403 part 3 - Test element becomes tainted by DrawImage;

https://reviewboard.mozilla.org/r/118412/#review120494

r+ for the .webidl
Attachment #8845219 - Flags: review?(bugs) → review+
Comment on attachment 8845218 [details]
Bug 1345403 part 2 - Mark element tainted when DrawImage is used;

https://reviewboard.mozilla.org/r/118410/#review120658
Attachment #8845218 - Flags: review?(matt.woodrow) → review+
Try looks very good, thanks for the review!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7a7d6e9b8329
part 1 - Track decoder tainting; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/ba072186c917
part 2 - Mark element tainted when DrawImage is used; r=jwwang,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/a3e4ce12194f
part 3 - Test element becomes tainted by DrawImage; r=jwwang,smaug
https://hg.mozilla.org/integration/autoland/rev/ac12e246a808
part 4 - Clean up suspend timer canceling; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f6415ae0ba89
part 5 - Test video suspend canceling; r=jwwang
Keywords: checkin-needed
kaku: This caused perma failures in Autophone's Mdm tests:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f6415ae0ba892b17b807c2dd1e0961140b1b80d0&filter-searchStr=autophone

Can you back this out and fix the issues or the tests before relanding?

You can test this on try with

try: -b do -p android-api-15 -u autophone-mochitest-media -t none --artifact
Flags: needinfo?(kaku)
I will then disable those newly add tests on Android and open bug 1346705 for tracking/fixing them.
Flags: needinfo?(kaku)
Blocks: 1346705
Try looks good, thanks for review and check in again!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2305bfbe6545
part 1 - Track decoder tainting; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/99e3488b1ea4
part 2 - Mark element tainted when DrawImage is used; r=jwwang,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7f9def21c5b7
part 3 - Test element becomes tainted by DrawImage; r=jwwang,smaug
https://hg.mozilla.org/integration/autoland/rev/850a10a1e44f
part 4 - Clean up suspend timer canceling; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/be9cfb690427
part 5 - Test video suspend canceling; r=jwwang
Keywords: checkin-needed
Depends on: 1347836
Depends on: 1347860
You need to log in before you can comment on or make changes to this bug.