-
Notifications
You must be signed in to change notification settings - Fork 31
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
vod doc rough draft init #1074
vod doc rough draft init #1074
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me, waiting to see code review results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level feedback:
- Overall, it covers the right topics.
- Needs some formatting work to make it more readable, including proper use of headers.
- There are some pretty blatant copywriting errors that should've gotten caught in a read-through. At least one section needs to be rewritten for clarity.
- I think we should add that we're working on webhooks (or include instructions for polling) to understand the status of the tasks more manageable. It feels a little strange to suggest that users keep manually trying.
- Both methods of uploading are transcoded now
|
||
# Video on Demand | ||
|
||
This is a good tutorial for someone who want to upload video to livepeer using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rephrase: This tutorial covers how to upload the video to Livepeer to allow viewers to playback stored video assets in your application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shih-yu resolved changes
@@ -0,0 +1,687 @@ | |||
--- | |||
title: Video On Demand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than call this Video On Demand, let's call it "Upload a Video" which is clearer to readers -- can you chance the associated URLs, menus, and descriptions to fall along these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shih-yu resolved changes
1. Import the asset - **only MP4 file format** | ||
2. Upload the asset - gets transcoded to MP4(H.264 + AAC) | ||
|
||
### **Importing assets** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've removed the distinction between "Importing" and "Uploading" assets in the API, changes will be merged 5/31/22. They can be renamed "Upload from URL" and "Upload Locally".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shih-yu resolved changes
|
||
## Assets | ||
|
||
When providing an asset for VOD, there are two options, either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"There are two methods to upload a video to Livepeer:
- Upload via URL, with an .mp4 file format
- Upload locally"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, both uploading via URL and uploading locally will be transcoded. The way it reads is that only the local upload will be transcoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shih-yu resolved changes
> _Note that the status is pending, it might take a few minutes for it to be | ||
> imported, to verify use | ||
> [`https://livepeer.com/api/asset/$ASSET_ID/`](https://livepeer.com/api/asset/$ASSET_ID/) | ||
> with provided [asset.id](http://asset.id) in the response from above_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this clear enough? Should we add code that developers can copy and paste more easily?
|
||
Use the list tasks method to get CID and metadata - | ||
|
||
To Get the CID and metadata URL for the asset that was just exported, you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph should be rewritten for clarity. Feel free to reach out to me for suggestions.
|
||
Caveats | ||
|
||
- At the moment, cannot edit or delete assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't true and needs to be added. @codentell can you reach out to @victorges to get these end points documented?
} | ||
``` | ||
|
||
Caveats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this section goes at the top of the article? These are good things to know before beginning to work with our VOD API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shih-Yu @codentell Please re-read through and make punctuation consistent. There are lots of unpunctuated sentences. Feel free to ask @hnjolles1 if you're unsure about any copywriting details.
|
||
## Caveats | ||
|
||
- Have to manually check the status of the file using `List Task` method to see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this is a funny thing to ask a developer to do -- we should either tell readers that we're currently working on webhooks (which feels like the obvious thing that users are going to want) or give them instructions to do polling. @victorges thoughts on this?
If we decide that this okay, it needs to be rephrased: Currently, users need to manually check the status of the file using List Task method (described below) to see if uploads have been completed. We're currently working on webhooks to make this easier for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I don't think this is a caveat worth noting here as a big issue in the flow. It's just how things work for now while we don't have a more sophisticated interface. Webhooks will be nicer for communicating the state to a backend, but it won't make much of a difference for users building only a frontend (or simpler backends without custom state) anyway.
Also agree that we must provide instructions for polling along this guide. Can also link to the video-nft
SDK (that needs rebranding) for an example on how to wait for a task to finish: https://github.com/livepeer/video-nft/blob/4e24bd46a884f7a0c23fe23b233cfd0c9a3bafad/src/minter.ts#L480
We can also create a simple gist
or code snippet for that using axios.
|
||
Use the list tasks method to get CID and metadata | ||
|
||
To Get the CID and metadata URL for the asset that was just exported, you can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to
To get the CID and metadata URL for the new, just created asset, retrieve the task until it is completed. To do this, check the status field in the response. This finished task will have an output with all the information about that asst, and the CID information is located under the output.export.ipfs
path.
How difficult is it to have the side bar menu reflect the major sections in this doc? Could make it easier for developers to find what they're looking for or to figure out if we have the right functionality. |
|
||
## Caveats | ||
|
||
- Have to manually check the status of the file using `List Task` method to see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I don't think this is a caveat worth noting here as a big issue in the flow. It's just how things work for now while we don't have a more sophisticated interface. Webhooks will be nicer for communicating the state to a backend, but it won't make much of a difference for users building only a frontend (or simpler backends without custom state) anyway.
Also agree that we must provide instructions for polling along this guide. Can also link to the video-nft
SDK (that needs rebranding) for an example on how to wait for a task to finish: https://github.com/livepeer/video-nft/blob/4e24bd46a884f7a0c23fe23b233cfd0c9a3bafad/src/minter.ts#L480
We can also create a simple gist
or code snippet for that using axios.
- File size is limited to 1GB. Any files greater than 1GB will error with | ||
NETWORK CHANGED. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of this error and do not believe that it will always error out with that. Would rephrase with
- File size is limited to 1GB. Any files greater than 1GB will error with | |
NETWORK CHANGED. | |
- Files are currently limited to 1GB in size. Any files greater than that will likely error out during the upload or processing steps. |
- Upload via URL, with an `.mp4` file format | ||
|
||
- Upload locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only support MP4 everywhere, and the caveat has already been made in the beginning. No need to reemphasize it here and only in one of the methods.
Can also improve the wording:
- Upload via URL, with an `.mp4` file format | |
- Upload locally | |
- Import from a URL | |
- Directly upload from a local file |
(we theoretically wanted to use "upload" on both cases but I don't really think it makes sense... The API is still called /import
anyway so it will be confusing not to use that here. @pglowacky do you really think we need to call it upload here? If so let me know and I can rename the API as well)
|
||
- `Upload locally` | ||
|
||
### Upload from URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be consistent with how we call the steps in the beginning here
- Get a | ||
[Livepeer Video Service API key](https://livepeer.com/docs/guides/start-live-streaming/api-key) | ||
|
||
## Assets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this section name makes sense.
- Provide the name of the asset | ||
- Provide the URL link of the asset that is to be downloaded | ||
|
||
JS + axios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just call this JavaScript
. We already made the disclaim in the beginning that we will be using axios
.
|
||
JS + axios | ||
|
||
```jsx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just js
?
axios | ||
.put("${url}", { | ||
headers: { | ||
"Content-Type": "video/mp4", | ||
}, | ||
data: "@$VIDEO_FILE_PATH", | ||
}) | ||
.then((res) => { | ||
console.log(JSON.stringify(res.data)); | ||
}) | ||
.catch((error) => console.log(error)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These code snippets will be much more useful if they build on top of each other. So instead of building independent snippets that can only be run like a script that gets all input from templated vars, the first snippet in this process should populate an url
variable which is then used here.
The video file should also not be sent as a string. If the idea is to read from a file, you should include here the code that opens a file and sends it as the axios
request payload. This can be as simple as fs.createReadStream(path)
and sending that as the request data
.
Also same regarding using async/await for cleanliness.
- Use the `https://livepeer.com/api/asset/$ASSET_ID/` with the provided | ||
asset.id\* to check that the video is uploaded, the status should be | ||
`"status": "ready"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not need to repeat this on both methods of uploading a file.
IMO should instead:
- introduce the "asset" and "task" resource types in the beginning of the documentation (around the current
## Assets
section) - Explain that adding a video file is an asynchronous process that requires processing the file. Because of that, we will first explain how to send the data to the API and then how to track the progress of the processing
- in each of the methods, specify when should the code grab the
asset
andtask
references/IDs (on the response of/import
and/request-upload
) - After both methods, explain how to monitor the progress of the tasks and then grab the final imported asset.
`“type”: “export”`. | ||
|
||
```json | ||
{ | ||
//Uploaded asset via URL | ||
"id": "8b0cd796-8be1-412b-b1dc-8251a35ad2e7", | ||
"type": "export", | ||
"output": { | ||
"export": { | ||
"ipfs": { | ||
"videoFileCid": "QmaMvRNEwvCHuwEZjX9NzFTJtBUrbk7j5D7ngP8xATHwXK", | ||
"nftMetadataCid": "QmaY8HqYV3kVxwsCQahMYisQiZnjZg2bRVjb6XspTmYMFi", | ||
"videoFileUrl": "ipfs://QmaMvRNEwvCHuwEZjX9NzFTJtBUrbk7j5D7ngP8xATHwXK", | ||
"videoFileGatewayUrl": "https://ipfs.livepeer.com/ipfs/QmaMvRNEwvCHuwEZjX9NzFTJtBUrbk7j5D7ngP8xATHwXK", | ||
"nftMetadataUrl": "ipfs://QmaY8HqYV3kVxwsCQahMYisQiZnjZg2bRVjb6XspTmYMFi", | ||
"nftMetadataGatewayUrl": "https://ipfs.livepeer.com/ipfs/QmaY8HqYV3kVxwsCQahMYisQiZnjZg2bRVjb6XspTmYMFi" | ||
} | ||
} | ||
}, | ||
"params": { | ||
"export": { | ||
"ipfs": {} | ||
} | ||
}, | ||
"status": { | ||
"phase": "completed", | ||
"updatedAt": 1652903908984 | ||
}, | ||
"userId": "eada599f-3d58-499b-ba24-c7f3faf988de", | ||
"createdAt": 1652903903963, | ||
"inputAssetId": "87289307-b60f-432b-984a-8ede174fa152" | ||
}, | ||
|
||
``` | ||
|
||
Pass in the task ID for the imported video and the response will show | ||
`“type”: “import”` | ||
|
||
```json | ||
{ | ||
//Uploaded asset via URL | ||
"id": "77588656-ab08-4dd2-a244-0ff8df7ce664", | ||
"type": "import", | ||
"output": { | ||
"import": { | ||
"assetSpec": { | ||
"hash": [ | ||
{ | ||
"hash": "6dad0a8e7a92d537c6305fe6ae1e1881", | ||
"algorithm": "md5" | ||
}, | ||
{ | ||
"hash": "6132f4e43472b56b4047b7ad018147056c0c2dba9306cd04504e9c8208e72257", | ||
"algorithm": "sha256" | ||
} | ||
], | ||
"name": "pexels-peter-fowler-6394054.mp4", | ||
"size": 32407849, | ||
"type": "video", | ||
"videoSpec": { | ||
"format": "mp4", | ||
"tracks": [ | ||
{ | ||
"fps": 23.976023976023978, | ||
"type": "video", | ||
"codec": "h264", | ||
"width": 4096, | ||
"height": 2048, | ||
"bitrate": 21324821, | ||
"duration": 12.011667, | ||
"pixelFormat": "yuv420p" | ||
}, | ||
{ | ||
"type": "audio", | ||
"codec": "aac", | ||
"bitrate": 253375, | ||
"channels": 2, | ||
"duration": 12.031667, | ||
"sampleRate": 48000 | ||
} | ||
], | ||
"duration": 12.031667 | ||
} | ||
} | ||
} | ||
}, | ||
"params": { | ||
"import": { | ||
"url": "https://www.pexels.com/video/6394054/download/" | ||
} | ||
}, | ||
"status": { | ||
"phase": "completed", | ||
"updatedAt": 1652894675639 | ||
}, | ||
"userId": "eada599f-3d58-499b-ba24-c7f3faf988de", | ||
"createdAt": 1652894665451, | ||
"outputAssetId": "87289307-b60f-432b-984a-8ede174fa152" | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of these belong in this guide. We can create proper API references for those (including request/response examples) in the API reference (and maybe link to them here), but they should not be part of a "upload a video" tutorial (which is also the only VOD guide).
I think all this should be replaced with the part of the guide that teaches how to monitor the progress of an upload instead.
Co-authored-by: Victor Elias <victorgelias@gmail.com>
Co-authored-by: Victor Elias <victorgelias@gmail.com>
Co-authored-by: Victor Elias <victorgelias@gmail.com>
Co-authored-by: Victor Elias <victorgelias@gmail.com>
Co-authored-by: Victor Elias <victorgelias@gmail.com>
Publishing for now (I keep getting questions about VOD and desperately need to be able to send documentation). @Shih-Yu @codentell Have all of Victor's comments been addressed? |
@pglowacky Only these two comments still need to be addressed: |
What does this pull request do? Explain your changes. (required)
Specific updates (required)
-
Does this pull request close any open issues?
Screenshots (optional):
Checklist: