Download video to sync to local cache#113
Conversation
local/local.go
Outdated
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() | ||
|
|
||
| metadataBytes, err := ioutil.ReadAll(f) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
i recommend os.ReadFile() to read the file in one go
|
Posting some work before I get too far off into the weeds. There are some comments about improvements that could be made to the go jsonrpc client, and stuff I still need to do for the upload. This is due for a refactor, I just wanted to get the whole process out into code before figuring out how to organize it. |
pseudoscalar
force-pushed
the
issue/112/v1
branch
from
bb45c12 to
eb30fa4
Compare
local/local.go
Outdated
| log.Debugf("Object to be published: %v", processedVideo) | ||
|
|
||
| } else { | ||
| done, err := publisher.Publish(*processedVideo) |
There was a problem hiding this comment.
id call this something other than done to make it clear its a channel that lets you know when the video is done reflecting
local/localSDKPublisher.go
Outdated
| break | ||
| } | ||
| if !fileStatus.UploadingToReflector { | ||
| log.Warn("Stream is not being uploaded to a reflector. Check your lbrynet settings if this is a mistake.") |
There was a problem hiding this comment.
if this is an error (or an unexpected thing), it should prolly send an error to done, right?
There was a problem hiding this comment.
The thinking was that this may be intentional since it is driven by lbrynet settings. I could turn this into an error if that case is unlikely. I could also add a command line flag to indicate that streams won't be reflected.
There was a problem hiding this comment.
yea id either make it an option or just assume that all streams will be reflected. option is better but more work
local/local.go
Outdated
| if len(description) > maxLength { | ||
| description = description[:maxLength] | ||
| } | ||
| return description + "\n..." + additionalDescription |
There was a problem hiding this comment.
minor nit, but doesn't this potentially make the description longer than the max length?
There was a problem hiding this comment.
This was mostly ripped from getAbbrevDescription in sources/youtubeVideo.go. I didn't know exactly where the value of 2800 came from, so I just assumed the logic was correct. If it is supposed to be a limit on the total description length, I'll fix this.
There was a problem hiding this comment.
i think that's an estimate of how long the description can get before we hit the max claim size limit. id still fix it so the math is right though :-)
|
|
||
| func NewYouTubeAPIVideoEnricher(apiKey string) (*YouTubeAPIVideoEnricher) { | ||
| enricher := YouTubeAPIVideoEnricher{ | ||
| api: NewYouTubeAPI(apiKey), |
There was a problem hiding this comment.
so the best way to get the release time was via the api? if so, can you include instructions in your PR on what a user would need to do to get an api key? we'd also want to add handling in a later version for exceeding api limits
There was a problem hiding this comment.
This was the simplest and most direct way, and the API limits seem like more than enough for the single channel use-case (except on initial channel sync). Should I add the instructions to the local/readme.md?
There was a problem hiding this comment.
maybe add a note to the issue to give the user a good error message if the api runs out of credits, and stop the sync. getting the release time right is fairly important so we don't want to guess or leave it empty. this could go under v4
| FullLocalPath: videoPath, | ||
| } | ||
|
|
||
| for _, enricher := range s.enrichers { |
There was a problem hiding this comment.
this is an interesting design. what other enrichers did you envision?
There was a problem hiding this comment.
This mainly came out of trying to accommodate easy switching between methods of getting release date and also allowing for fallback to multiple methods. The other methods in mind were things like the odysee API, multiple YouTube API keys, a future update to yt-dlp that might provide it, web scraping (although I don't think this would currently work as only the date is provided), or some 3rd party metadata provider. In the single video use-case, there could even be an enricher that just supplies values specified on the command line.
The design here is meant to put together a list of enrichers based on command line options. The enrichers will then take turns trying fill-in the missing video metadata (although right now, it is only trying to fill-in release date)
… YouTube API key. Fix some minor issues.
2 participants
Partial completion of v1 of issue #112. Drafting this PR for discussion.