Skip to content
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

Add Azure blob storage backend #638

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

joshspicer
Copy link

finally getting around to #505 😅

Copy link
Collaborator

@aspacca aspacca left a comment

Choose a reason for hiding this comment

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

hello @joshspicer , thank you very much for the contribution!

much appreciated!

please see my comments and we can probably merge this soon

@joshspicer joshspicer requested a review from aspacca February 2, 2025 00:53
Copy link
Collaborator

@aspacca aspacca left a comment

Choose a reason for hiding this comment

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

i approve the PR, please just test the content range and change the service url that's currently hard-coded only if my question has a positive answer :)

send a final comment as feedback and i'm going to merge if nothing else is left!

thank you again for the contribution

key := fmt.Sprintf("%s/%s", token, filename)
blobClient := s.containerClient.NewBlobClient(key)

var options *azblob.DownloadStreamOptions
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 🥇

can you please make sure that content range works? you can test uploading a video an moving fast fowarded and skipping back and forth in the video player on the donwload page :)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the idea of how to test, I wasn't familiar with content range before this. I'll give it a go soon and share the result 😃

Copy link
Author

Choose a reason for hiding this comment

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

I'm able to see the header with your technique but this then results in an error (and the video does not seek forward)

image

I will take a closer look later. If you have suggestions on where I should place some breakpoints, let me know :)

Copy link
Collaborator

@aspacca aspacca Feb 5, 2025

Choose a reason for hiding this comment

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

there's a context canceled that should mean that the request to get the blob is timing out.

my guess is that the problem is that you pass rng.Limit as the azblob.HTTPRange.Count, that in this case is 0, and this might mess up the request, leading it to time out (instead of a more proper bad request status return).

it can't be that you have to set azblob.HTTPRange.Count only if rng.Limit > 0, because it is not a pointer, so the zero value is already 0, but it might something like for storj:

if rng.Limit > 0 {
options.Length = int64(rng.Limit)
} else {
options.Length = -1
}

i suggest to give it a try with the same if logic and -1 (is azblob.HTTPRange.Count unsigned or not?), if it doesn't work let's look in the azblob documentation if anything is specified. if not, i guess we'd have to file a bug to azure for that.

do you have access to some kind of azure support by chance? if so you might consider asking support (directly or after the test and looking at the docs ;))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

unless you are taking it form an HEADrequest :)

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps I need a larger video, but with 8 minute videos i'm not hitting a breakpoint inside the if rng != nil {...} block. That said, the experience is ok

Screen Recording 2025-02-17 at 7 25 42 PM

Copy link
Author

Choose a reason for hiding this comment

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

Tested with a 20 minute video too and same as above (scrubs nicely but no range being used in the GET).

I also tried hard-coding in an offset, but it appears you need to pick the offsets carefully or else you corrupt the video?

image

I removed the range handling code, perhaps we can look at it at a later date?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps I need a larger video, but with 8 minute videos i'm not hitting a breakpoint inside the if rng != nil {...} block

and scrubbling works anyway?
can you check your network tab and see if any GET request with a content-range header is done?

let's double check the range issue one last time then i'll review the final code and approve (it seems nothing is left, at the moment)

Copy link
Author

Choose a reason for hiding this comment

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

scrub.mov

Tried from b8c00d8

Copy link
Collaborator

@aspacca aspacca left a comment

Choose a reason for hiding this comment

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

oki, sorry, asking for a little few more changes :)

please check my latest comments 🙏 :

The Head() request needs us to still craft a bloc client to retrieve
the ContentLength. Otherwise, we can use the helpers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants