Skip to content

Add two demos using streams for PNG manipulation #911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 1, 2018
Merged

Conversation

ksm2
Copy link
Contributor

@ksm2 ksm2 commented Mar 27, 2018

I added two demos I wrote for MDN to the list of your demos, if you'd like!

@ricea
Copy link
Collaborator

ricea commented Mar 28, 2018

Great stuff!

  • I'd like GrayscalePNGSource to be a transform stream. Wrapping a ReadableStream inside another ReadableStream is a pattern I'm trying to discourage. I realise the fact that no browser has shipped TransformStream yet makes this inconvenient! If it's not possible to change this, could we link only to "Unpack chunks of a PNG" for the time being?
  • In logReadableStream(), it would be much easier to create a WritableStream and then pipe to it. Basically the contents of pump() would instead be in the write() method, except for the if (done) part which would go in the close() method. Try it, I think you'll like it!

As a general rule, any time you're using getReader() or getWriter(), you should think about whether there would be a simpler way to do it with pipeTo() or pipeThrough().

General comments:

  • It looks like GrayscalePNGSource won't work properly unless the input is an rgba PNG. Maybe it should throw in other cases?
  • Using
const {value, done} = await reader.read();

inside a loop in an async function is much easier and cleaner that using a pump() function. I think all the browsers that have or will have ReadableStream already support async/await.

It took me a while to understand what "Unpack chunks of a PNG" was doing. It would probably benefit from a bit more documentation.

@ksm2
Copy link
Contributor Author

ksm2 commented Mar 28, 2018

I have seen TransformStream in the spec, but without the browser support I felt not encouraged enough to build the whole demo against it if I cannot test the result in at least Chrome. Is there some kind of shim I could use to patch TransformStreams to browsers which support ReadableStream and WritableStream?

You could not make me happier with your statement about async/await! Actually, I wrote the whole code with async functions first as I am a huge fan of the concept – but MDN disapproved it fo being to complex to newbies of ES6+. (Actually, in the case of streams, I hope for for-await-of to enter ECMAScript as this would be the easiest way to iterate over a ReadableStream IMHO.)

Should I make the code changes here in the spec's repository in the demo directory? I also just filed a PR against MDN's repository. I am not shure they support changes there.

@ricea
Copy link
Collaborator

ricea commented Mar 29, 2018

I have seen TransformStream in the spec, but without the browser support I felt not encouraged enough to build the whole demo against it if I cannot test the result in at least Chrome. Is there some kind of shim I could use to patch TransformStreams to browsers which support ReadableStream and WritableStream?

It's available in Chrome with the experimental web-platform-features flag enabled: chrome://flags/#enable-experimental-web-platform-features

It will hopefully be turned on by default in version 67.

https://streams.spec.whatwg.org/demos/transforms/transform-stream-polyfill.js is the outdated but mostly compatible polyfill used by the demos linked from the standard.

Actually, in the case of streams, I hope for for-await-of to enter ECMAScript as this would be the easiest way to iterate over a ReadableStream IMHO.

Hopefully you'll be able to use this sooner rather than later: #778

Should I make the code changes here in the spec's repository in the demo directory? I also just filed a PR against MDN's repository. I am not shure they support changes there.

If we cannot find a compromise with MDN then that would be my preference, yes.

@ksm2
Copy link
Contributor Author

ksm2 commented Apr 19, 2018

If we cannot find a compromise with MDN then that would be my preference, yes.

Hey @ricea, the folks at MDN merged my updates now using TransformStreams!

demos/index.html Outdated
@@ -25,6 +25,12 @@ <h1>Streams Demos</h1>

<dt><a href="https://fetch-progress.anthum.com/">Progress indicators with Service Workers and Fetch</a> | <a href="https://github.com/AnthumChris/fetch-progress-indicators">(Source Code)</a>
<dd>Streams intercept Service Worker <code>FetchEvent</code> and <code>fetch()</code> statements to show progress indicators. See <a href="https://github.com/AnthumChris/fetch-progress-indicators#browser-support">browser support</a> notes.

<dt><a href="http://mdn.github.io/dom-examples/streams/grayscale-png/">Grayscale a PNG</a> | <a href="https://github.com/mdn/dom-examples/tree/master/streams/grayscale-png">(Source Code)</a>
<dd>A PNG gets fetched to receive a ReadableStream on it, than the PNG data is converted to black and white and enqueued into another ReadableStream.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this comment to indicate that it uses TransformStream?

Also, in the demo itself, it would nice if it displayed a message in the page when TransformStream is not defined, to avoid confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to block this on the changing http://mdn.github.io/dom-examples/streams/grayscale-png/. The MDN people might not agree to that anyway.

@ricea ricea merged commit af48a18 into whatwg:master May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants