Less algorithms, better method/dfn linking #112

chcunningham · 2020-12-09T07:53:58Z

This flattens most of the codec algorithms into the methods where they were being called. For instance, instead of a "Configure Decoder" algorithm, we now simply have those steps listed under AudioDecoder.configure() and VideoDecoder.configure(). This does mean more copy/paste, but its much more readable for having less jumping around the document and needing less parameterization (e.g. AudioDecoder.configure() can easily refer to AudioDecoder's interface members).

I was on the fence about this vs another approach (#111) where I added more parameters to the algorithms in an attempt to get better linking for where things are used. Having done both, I definitely prefer this option. The other approach is too clever. Code re-use is great, but this is a spec so readability is more important.

Sorry this is fairly large. I did at least break it up into distinct commits for converting each of the interfaces. Also, once you read one the others are pretty obvious.

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Feb 8, 2021, 11:11 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.

🔗 Related URL

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>500 Internal Server Error</title>
</head><body>
<h1>Internal Server Error</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator at 
 [no address given] to inform them of the time this error occurred,
 and the actions you performed just before this error.</p>
<p>More information about this error may be available
in the server error log.</p>
<hr>
<address>Apache/2.4.10 (Debian) Server at api.csswg.org Port 443</address>
</body></html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

padenot

This needs more work. I only looked at the first patch, but you told me the others are similar. The processing model section and its use feel off, either the thread on which the actual codec operation is implied, but then it's missing the fact that things need to run "in parallel", or it's all running synchronously. It must be the former.

index.src.html

padenot · 2020-12-10T15:36:31Z

index.src.html

-    2. Run the <a>Configure Decoder</a> algorithm with |config|.
+    2. If {{AudioDecoder/state}} is `“closed”`, throw an {{InvalidStateError}}.
+    3. If the user agent cannot provide a codec implementation to support
+        config, throw a {{NotSupportedError}}.


This needs to be a bit less hand-wavy. It's useful to tell authors why it failed (hardware resources exhausted, resolution too big, the device/browser doesn't know how to do this, etc.). It seems like after #98, we could just refer to the concept it will introduce.

This is tricky. There are a (growing) number of config options that could theoretically cause a failure. Sometimes multiple options may be equally to blame. Maybe we can define some common cases?

My hope is that the simple rejection is no less user friendly. I imagine apps will tend to walk an ordered list of configurations based on their content library (decoding) or the remote users capabilities (rtc encoding). If we tell them "hardware resources exhausted", they probably handle that the same as "unsupported codec".

Re: #98, see #120

index.src.html

chcunningham

Thanks Paul!

index.src.html

chcunningham · 2020-12-15T07:16:14Z

index.src.html

-    2. Run the <a>Configure Decoder</a> algorithm with |config|.
+    2. If {{AudioDecoder/state}} is `“closed”`, throw an {{InvalidStateError}}.
+    3. If the user agent cannot provide a codec implementation to support
+        config, throw a {{NotSupportedError}}.


This is tricky. There are a (growing) number of config options that could theoretically cause a failure. Sometimes multiple options may be equally to blame. Maybe we can define some common cases?

My hope is that the simple rejection is no less user friendly. I imagine apps will tend to walk an ordered list of configurations based on their content library (decoding) or the remote users capabilities (rtc encoding). If we tell them "hardware resources exhausted", they probably handle that the same as "unsupported codec".

index.src.html

chcunningham

Thanks Paul!

index.src.html

chcunningham

Thanks Paul. Holding off on new commits until we wrap the discussion on parallel queues.

index.src.html

So far only AudioDecoder is updated. I'll update the other interfaces pending feedback.

chcunningham

Thanks again! I've resolved a number of issues we discussed over VC. As planned, the latest commits start the transition to explicit threads. I'll follow up with migrating other interfaces after review. Also, there are a few small pieces of your feedback I still plan to incorporate - out of time tonight.

index.src.html

Makefile

index.src.html

padenot

Generally this looks exactly what we need, I put in some comments, and do a last pass when the various usage of this pattern will be written.

index.src.html

padenot · 2021-01-26T18:02:07Z

index.src.html

+The <dfn>control thread</dfn> is the thread from which authors will construct
+and [=codec=] and invoke its methods. Invoking a codec's methods will typically
+result in the creation of [=control messages=] which are later executed on the
+[=codec thread=]. Each [=JavaScript realm=] has a separate control thread.


I can never remember if it's best to talk in terms for global or realms, and why. I seem to see globals more often in this kind of spec (non ecmascript).

I've just applied s/Javascript realm/global object/. LMK if this is how you intended it to read. I tried to make sense of https://html.spec.whatwg.org/#realms-settings-objects-global-objects but it's very hard to grok.

index.src.html

…ss a list

chcunningham

Thanks Paul. LMK if this looks good and I'll apply it to the other interfaces.

chcunningham · 2021-01-27T00:29:01Z

index.src.html

+The <dfn>control thread</dfn> is the thread from which authors will construct
+and [=codec=] and invoke its methods. Invoking a codec's methods will typically
+result in the creation of [=control messages=] which are later executed on the
+[=codec thread=]. Each [=JavaScript realm=] has a separate control thread.


I've just applied s/Javascript realm/global object/. LMK if this is how you intended it to read. I tried to make sense of https://html.spec.whatwg.org/#realms-settings-objects-global-objects but it's very hard to grok.

index.src.html

chcunningham

I've applied the feedback to the other codec interfaces and pushed some of the outstanding issues into follow up items (tightening scope to improve velocity). I think we're nearly there! A few open comments waiting for your reply/resolve.

index.src.html

padenot

Probably good to merge, and we can discuss the two points I added here as followups. Thanks!

index.src.html

chcunningham · 2021-02-08T23:27:28Z

Thanks a ton!

chcunningham added 4 commits December 8, 2020 22:59

Refactor AudioDecoder algorithms and methods

918aa39

Refactor VideoDecoder algorithms and methods

cb4519d

Refactor AudioEncoder algorithms and methods

41e46c9

Refactor VideoEncoder algorithms and methods

e7d3535

chcunningham requested a review from padenot December 9, 2020 07:53

chcunningham mentioned this pull request Dec 9, 2020

Add explicit arguments to codec algorithms. #111

Closed

padenot requested changes Dec 11, 2020

View reviewed changes

padenot feedback round 1

afbce3d

chcunningham commented Dec 15, 2020

View reviewed changes

padenot feedback round 2

62db336

padenot reviewed Jan 11, 2021

View reviewed changes

padenot feedback round 3

412dcf2

chcunningham commented Jan 12, 2021

View reviewed changes

chcunningham commented Jan 15, 2021

View reviewed changes

index.src.html Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

chcunningham added 2 commits January 25, 2021 22:10

Add thread defs to processing model, update AudioDecoder.

5e169a8

So far only AudioDecoder is updated. I'll update the other interfaces pending feedback.

reset queue size variable as part of reset algorithms

e208ada

chcunningham commented Jan 26, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

index.src.html Show resolved Hide resolved

chcunningham commented Jan 26, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

padenot reviewed Jan 26, 2021

View reviewed changes

various feedback, notably changing audiodec output algorithm to proce…

9f8383e

…ss a list

chcunningham commented Jan 27, 2021

View reviewed changes

Update remaining codec interfaces w/ feedback

c287274

chcunningham mentioned this pull request Feb 1, 2021

Improve reset algorithm clarity #128

Open

chcunningham commented Feb 1, 2021

View reviewed changes

index.src.html Show resolved Hide resolved

index.src.html Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

update encodeQueueSize to match decodeQueueSize

3cd2b4c

padenot approved these changes Feb 8, 2021

View reviewed changes

index.src.html Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

chcunningham mentioned this pull request Feb 8, 2021

Copy [[pending promises]] wording from WebAudio #136

Closed

chcunningham added 2 commits February 8, 2021 15:04

Close() wording feedback

4f96e81

Merge branch 'master' into less_algorithms

72457ae

chcunningham merged commit bb5e4b5 into master Feb 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less algorithms, better method/dfn linking #112

Less algorithms, better method/dfn linking #112

chcunningham commented Dec 9, 2020 •

edited by pr-preview bot

Loading

padenot left a comment

padenot Dec 10, 2020

chcunningham Dec 15, 2020

chcunningham Dec 30, 2020

chcunningham left a comment

chcunningham Dec 15, 2020

chcunningham left a comment

chcunningham left a comment

chcunningham left a comment

padenot left a comment

padenot Jan 26, 2021

chcunningham Jan 27, 2021

chcunningham left a comment

chcunningham Jan 27, 2021

chcunningham left a comment •

edited

Loading

padenot left a comment

chcunningham commented Feb 8, 2021

Less algorithms, better method/dfn linking #112

Less algorithms, better method/dfn linking #112

Conversation

chcunningham commented Dec 9, 2020 • edited by pr-preview bot Loading

💥 Error: 500 Internal Server Error 💥

padenot left a comment

Choose a reason for hiding this comment

padenot Dec 10, 2020

Choose a reason for hiding this comment

chcunningham Dec 15, 2020

Choose a reason for hiding this comment

chcunningham Dec 30, 2020

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

chcunningham Dec 15, 2020

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

padenot left a comment

Choose a reason for hiding this comment

padenot Jan 26, 2021

Choose a reason for hiding this comment

chcunningham Jan 27, 2021

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

chcunningham Jan 27, 2021

Choose a reason for hiding this comment

chcunningham left a comment • edited Loading

Choose a reason for hiding this comment

padenot left a comment

Choose a reason for hiding this comment

chcunningham commented Feb 8, 2021

chcunningham commented Dec 9, 2020 •

edited by pr-preview bot

Loading

chcunningham left a comment •

edited

Loading