Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: Allow ipfs.files.cat to return slices of files #1231

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Feb 20, 2018

This PR exposes the begin/end arguments from ipfs-inactive/js-ipfs-unixfs-engine#207 in the ipfs.files.cat* methods to return slices of files. It also includes an example of how to stream video from ipfs to the browser using the videostream module.

Requires ipfs-inactive/js-ipfs-unixfs-engine#207 to be merged first.

@daviddias
Copy link
Member

daviddias commented Feb 20, 2018

@achingbrain this is phenomenal. Lot's of great stuff in this example, in fact, some of it should be merged into core.

It has been requested to have the offset flag for the cat command, see:

This should be something that is provided by the unixfs-engine itself. Would you like to work on that?

@daviddias daviddias changed the title docs: Add browser example for ReadableStreams docs: Add browser example for videostream with Readable Streams Feb 21, 2018
@achingbrain
Copy link
Member Author

Sure, I'll take a look :)

@mitra42
Copy link

mitra42 commented Mar 8, 2018

I think - though I’m not certain - that there is a mistake in offsetStream line 189 in if (endByte && endByte < streamPosition)

The parameters to this function aren’t commented (would be good to) but endByte appears to be the byte AFTER the one you want, since you’ve incremented it at line 89 in playByLinks end = opts.end ? opts.end + 1 : undefined

If I understand the

Note I have NOT checked the rest of the code in case the same problem exists elsewhere.

By the way, as an example it would be super useful to comment the parameters to functions, since as I’m trying to modify it for our needs I’m having to guess some of what you are doing and because of the lack of comments it took me a while to figure out the test above didn’t make sense.

@paralin
Copy link

paralin commented Mar 9, 2018

@mitra42 if you are working on cleaning this up into a proper bundle and are interested in open sourcing that effort, I'd be happy to contribute

@mitra42
Copy link

mitra42 commented Mar 10, 2018

@paralin What we are doing (at archive.org) is open-source, but I haven't pushed it to our repo because I haven't been able to get my version of this to work.

I'm trying to use the example to fix the problems we were having using IPFS streams with the Video tag, which means I'm having use a modified version of this code in order to get the asynchronicity to work.

I'm using playByLinks which looks like the right approach, (the other versions seem far too inefficient for the

The biggest issue is that the ipfs.object.links(hash) works and retrieves what looks like the right data structure (right sizes etc) but then calling await ipfs.object.data(link.multihash) doesn't work, its passing a Uint8Array which looks correct, but the promise always resolves to undefined. and an error Not allowed to load local resource: blob:null/f8bc2088-5cc0-4a65-8a31-01e5cf8cc302 is getting posted from somewhere - though we've seen that error in other situations without a problem.

I'm not using object.data anywhere else in my code, so I don't know if the code in this example is working (implying I've made a mistake adapting) or if the example also fails in which case maybe we've both made a mistake, or the js-ipfs docs for this function aren't correct.

Are you able to get the example here to work ?

@paralin
Copy link

paralin commented Mar 11, 2018

Hash: f85d3b29c238602558cc
Version: webpack 3.11.0
Time: 17838ms
        Asset     Size  Chunks                    Chunk Names
    bundle.js  2.26 MB       0  [emitted]  [big]  main
bundle.js.map  6.71 MB       0  [emitted]         main
   index.html  1.17 kB          [emitted]         
   [4] (webpack)/buildin/global.js 509 bytes {0} [built]
  [31] (webpack)/buildin/module.js 517 bytes {0} [built]
 [364] multi ./index.js 28 bytes {0} [built]
 [365] ./index.js 6.05 kB {0} [built]
 [481] (webpack)/buildin/amd-options.js 82 bytes {0} [built]
    + 894 hidden modules
Child html-webpack-plugin for "index.html":
     1 asset
       [0] ./node_modules/html-webpack-plugin/lib/loader.js!./index.html 1.57 kB {0} [built]
       [2] (webpack)/buildin/global.js 509 bytes {0} [built]
       [3] (webpack)/buildin/module.js 517 bytes {0} [built]
        + 1 hidden module
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (60) SSL certificate problem: unable to get local issuer certificate
More details here: https://curl.haxx.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.
error An unexpected error occurred: "Command failed.
Exit code: 60
Command: sh
Arguments: -c npm run build && curl https://www.html5rocks.com/en/tutorials/video/basics/devstories.mp4 -o
 dist/video.mp4 && http-server dist -a 127.0.0.1 -p 8888                                                 

Doesn't seem to work at the moment, due to some SSL issues (happens just after I run webpack)

@mitra42
Copy link

mitra42 commented Mar 11, 2018

Looks like a different problem than the one I'm struggling with.

@mitra42
Copy link

mitra42 commented Mar 11, 2018

In isolating the test, I seem to have discovered a problem with files.cat and object.links/object.data which I've posted as #1258 so the problem might not be in the code of this example.

@achingbrain
Copy link
Member Author

FWIW most of the code in this example will go away once ipfs-inactive/js-ipfs-unixfs-engine#202 is merged as it moves the offset stream logic into core.

@mitra42
Copy link

mitra42 commented Mar 11, 2018

@achingbrain - looks like ipfs-inactive/js-ipfs-unixfs-engine#202 will probably need some glue as well to work with a video tag.

@ghost ghost assigned achingbrain Mar 20, 2018
@ghost ghost added status/in-progress In progress and removed status/ready Ready to be worked labels Mar 20, 2018
@achingbrain achingbrain changed the title docs: Add browser example for videostream with Readable Streams feat: Allow ipfs.files.cat to return slices of files Mar 21, 2018
@achingbrain
Copy link
Member Author

The scope of this PR has changed a bit so I've updated the title & description to reflect that.

@mitra42 could you elaborate a bit? What extra glue is required?

@mitra42
Copy link

mitra42 commented Mar 21, 2018

@achingbrain - I’m a little confused, because my comment #1231 (comment) above, referred to some code which was a demo of 3 or 4 ways to pull a slice of a file, including one that used obj.links and obj.data (which I copied and adapted), another that read using files.cat and ignored earlier bytes and so on.

Strangely I can’t find that base code in this PR any more, so either I made this bug report on the wrong pull request, or the commit got replaced which I didn’t think possible? I’d think I’d been hallucinating, but I coped the code and made a bug report on it 😃

My comment about glue refers to whatever code we used having to be attached to an (existing) video tag, I didn't see any code that would enable you to do that in ipfs-inactive/js-ipfs-unixfs-engine#202 (and wasn't expecting any) but I now see an example in this commit based on videostream and that was the kind of code I was referring to.

We actually are using the RenderMedia package which wraps video stream and needs to be passed a createReadStream function in the same form as you do here, ( https://nodejs.org/api/fs.html#fs_fs_createreadstream_path_options - in particular an {start, end} parameter where end is the last byte wanted), so that should work fine.

Should we expect the change to catReadableStream to land in js-ipfs v0.28.3 or is it further away?

@achingbrain
Copy link
Member Author

Aha, ok - this PR has been evolving over time a little so the confusion is understandable, sorry about that. A lot of the code has moved into core so hopefully the example is now a bit easier to follow at least.

I'm hoping it'll be in the next release but it depends on when people get the time to review it.

@achingbrain achingbrain force-pushed the add-readablestream-example branch 3 times, most recently from b76eb50 to 9f5b933 Compare March 27, 2018 14:07
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

It is missing the HTTP-API endpoint update and the CLI command as well.

@achingbrain achingbrain force-pushed the add-readablestream-example branch 2 times, most recently from b9b2564 to 9ff4b79 Compare April 10, 2018 14:44
@achingbrain
Copy link
Member Author

@diasdavid http/cli commands should be there now but needs ipfs-inactive/js-ipfs-http-client#734 to be merged

@alanshaw
Copy link
Member

Would be good to get this and ipfs-inactive/js-ipfs-http-client#734 merged as the interface-ipfs-core PR has already been merged

@achingbrain
Copy link
Member Author

ipfs-inactive/js-ipfs-unixfs-engine#209 needs to be merged too

@daviddias
Copy link
Member

@achingbrain what's missing on this one?

@daviddias daviddias mentioned this pull request Apr 30, 2018
30 tasks
@achingbrain
Copy link
Member Author

@diasdavid It's waiting for js-ipfs-api to be released

@daviddias
Copy link
Member

@achingbrain please rebase master onto this branch. you should be fully unblocked.

feat: Allows for byte offsets when using ipfs.files.cat and friends to request slices of files
@achingbrain
Copy link
Member Author

Looks like this needs #1319 merged first. And #1319 needs this PR merged first. What fun!

@daviddias
Copy link
Member

@achingbrain got it (the problem when PRs dangle for too long) We can solve it by merging both PRs into one.

@daviddias daviddias changed the base branch from master to fix/files.add-pull-streams May 1, 2018 13:04
@ghost ghost assigned daviddias May 1, 2018
@daviddias daviddias merged commit fff0aa1 into ipfs:fix/files.add-pull-streams May 1, 2018
@ghost ghost removed the status/in-progress In progress label May 1, 2018
@parkan
Copy link
Contributor

parkan commented Jun 5, 2018

@achingbrain looks like ipfs-inactive/js-ipfs-unixfs-engine#207 (which appears to supersede ipfs-inactive/js-ipfs-unixfs-engine#202) got merged, does that mean the example in here no longer reflect the correct/most succinct API?

@achingbrain achingbrain deleted the add-readablestream-example branch June 6, 2018 07:20
@achingbrain
Copy link
Member Author

@parkan take a look at the example in https://github.com/ipfs/js-ipfs/tree/master/examples/browser-readablestream - it shows how to use ipfs.files.catReadableStream with the offset and length options. ipfs.files.catPullStream and ipfs.files.cat also support the same options for streaming/returning part of files.

@mitra42
Copy link

mitra42 commented Jun 6, 2018

Is that in a released version now?

@parkan
Copy link
Contributor

parkan commented Jun 6, 2018

@mitra42 looks like v0.29.* should have it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants