Bug 189805 – [MSE] Fix comparsion with uninitialized greatestDecodeDuration

• Component: Media
• Status: RESOLVED FIXED

[Alicia Boya García]

greatestDecodeDuration was being compared without being set to a valid value before... which resulted in it never being set to valid value... which resulted in distant appends being considered as part of the same coded frame group... which resulted in unwanted frame deletion for frames in the middle... which resulted in the `Append*OutOfOrder` YouTube TV tests failing (and likely seeks resembling the same sequence of appends failing too).

This patch fixes that issue and adds a mock test to ensure it does not happen again.

[Alicia Boya García]

Created attachment 350272 [details]
Patch

[Michael Catanzaro]

Comment on attachment 350272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350272&action=review

I'm definitely not qualified to review the new test, but since this is urgent, I'll just happily note that there is a new test, and I like new tests. Bonus points if you can get a multimedia reviewer to look at this tomorrow morning.

Be sure to add it to https://trac.webkit.org/wiki/WebKitGTK/2.22.x immediately after it lands.

> Source/WebCore/ChangeLog:3
> +        [MSE] Fix comparsion with uninitialized greatestDecodeDuration

comparison

[Xabier Rodríguez Calvar]

r=me

[WebKit Commit Bot]

Comment on attachment 350272 [details]
Patch

Clearing flags on attachment: 350272

Committed r236314: <https://trac.webkit.org/changeset/236314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44678604>

[Jer Noble]

A quick comment on the test itself. I added a function to video-test.js, waitFor(target, eventName) returning a promise. I’ve found that it lets me write more readable tests using async functions. I.e.:


Video.srcObject = mediaSource;
await waitFor(mediaSource, ‘sourceopen’);

sourceBuffer = mediaSource.appendSourceBuffer(....);
await waitFor(sourceBuffer, ‘updateend’);

Most tests written this way can end up being written as a single async function, and it ends up being much more clear at a glance what the function is doing.

[Alicia Boya García]

(In reply to Jer Noble from comment #7)
> A quick comment on the test itself. I added a function to video-test.js,
> waitFor(target, eventName) returning a promise. I’ve found that it lets me
> write more readable tests using async functions. I.e.:
> 
> 
> Video.srcObject = mediaSource;
> await waitFor(mediaSource, ‘sourceopen’);
> 
> sourceBuffer = mediaSource.appendSourceBuffer(....);
> await waitFor(sourceBuffer, ‘updateend’);
> 
> Most tests written this way can end up being written as a single async
> function, and it ends up being much more clear at a glance what the function
> is doing.

Thanks for that, I noticed later working on another test and indeed `waitFor()` is very useful; I'm glad to be able to use async-await in tests.