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

Video Resumable uploader #489

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@YuanhuiChen

YuanhuiChen commented Aug 17, 2015

Finally comes another update. Sorry I opened a new branch. You cannot see the old comments. I committed to some others' diff by mistake in the old branch.
The new changes:

  1. add integration test
  2. distinguish between no resumable exception and resumable exception in the transfer phase.
  3. fix some other issues.

Thanks again for your patience!

Demi Chen
@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Aug 17, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@gfosco

This comment has been minimized.

Contributor

gfosco commented Aug 18, 2015

This is great, and I'm willing to accept it as is. I wish it could go against version 5, but I guess that wasn't possible. Will leave it here for another day to give @SammyK & @yguedidi a chance to comment/nit if they want.

public function uploadVideo($target, $pathToFile, $accessToken, $maxTransferTries = 5)
{
$uploader = $this->getResumableUploader($accessToken, $maxTransferTries);
$endpoint = '/'.$target.'/videos';

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

We shouldn't auto-suffix the endpoint with videos since it might not always be that and none of the other parts of the SDK auto-suffix endpoints. :)

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

@SammyK uploading a video can only be done with an endpoint formated like /{node-id}/videos, so I think it's OK here, end-user will only need to give the node id.

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

@yguedidi I see what you're saying. I'm also concerned that users will be reading the Graph docs and then try to enter the endpoint directly here. I for one wouldn't assume the SDK would add the /videos suffix for me. So I'm still against auto-suffixing. :)

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

I'd prefer an easy to use signature with a good doc rather an opened signature to any endpoint with a pattern check in the definition :)

This comment has been minimized.

@codingtmd

codingtmd Aug 19, 2015

Personally, I'm with @yguedidi here. This uploader is designed for uploading videos. If we specify clear that 'target' means user/page id in the doc, user won't pass in a full endpoint path in.

*/
public function start($endpoint, $filePath)
{
$fileSize = filesize($filePath);

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

We should add a method to FacebookFile called getSize() that would do this. And then pass in the instantiation of FacebookVideo instead of $filePath here. :)

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

Why not :)
It's depend how we want this class API be.

$startReqParams
);
$res= $this->client->sendRequest($request)->getDecodedBody();

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

Space after $res. Also if sendRequest() throws this is going to be problematic as a chained method call.

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

@SammyK it shouldn't be problematic as a throw stop the execution :)

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

@yguedidi Oh true. :)

* @throws FacebookResumableUploadException
* @throws FacebookSDKException
*/
public function transfer($endpoint, FacebookTransferChunk $chunk)

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

This method needs to be refactored a bit. Maybe @yguedidi has some thoughts on extracting out some of this logic to other methods. Maybe after it gets merged we can play with refactoring it a bit before tagging it.

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

As a service, this class shouldn't be aware of max transfer tries (a call to transfer should just send the request to transfer a chunk).
Then, the max transfer tries part should go in the Facebook class, in the do...while loop. (and thanks to the code extraction proposed by my above comment, no code duplication)

This comment has been minimized.

@codingtmd

codingtmd Aug 19, 2015

Not sure whether we are in the same page. I feel that this max tries logic should be moved into the do...while loop of function resumeUpload. @yguedidi, is it what you mean?

This comment has been minimized.

@yguedidi

yguedidi Aug 20, 2015

Collaborator

Yes, more precisly in the newly extracted function with the do...while loop in it.

{
$fileUploader = new FacebookResumableUploader($this->fbApp, $this->fbSuccessClient, 'access_token');
$endpoint = '/113582528973300/videos';
$filePath = __DIR__ . '/../foo.mp4';

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

I still don't think we need to include an actual video file in the SDK. We can mock this file. (Also another reason we need to pass in the FacebookVideo instance instead of the path to the file.)

This comment has been minimized.

@codingtmd

codingtmd Aug 20, 2015

From the code, looks we only need to mock one function 'getSize', right? @YuanhuiChen, let us know if there is challenge here.

This comment has been minimized.

@SammyK

SammyK Aug 20, 2015

Collaborator

@codingtmd We just need to mock a FacebookVideo and inject it instead of using the path to the file. :)

*
* @return FacebookResumableUploader
*/
protected function getResumableUploader($accessToken, $maxTransferTries = 5)

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

Can we rename this method to getResumableUploadClient?

*/
protected function getResumableUploader($accessToken, $maxTransferTries = 5)
{
return new FacebookResumableUploader($this->app, $this->client, $accessToken, $maxTransferTries);

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

Can we rename this service to FacebookResumableUploadClient? :)

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

I will just name it FacebookVideoUploader :)

This comment has been minimized.

@SammyK

SammyK Aug 18, 2015

Collaborator

You would! :) I was just thinking since it's an HTTP client, it'd make more since to make it more descriptive, but I'm cool with what you guys think too :)

This comment has been minimized.

@codingtmd

codingtmd Aug 19, 2015

Either FacebookVideoUploader or FacebookVideoUploadClient is fine to me. The reason is, inside the function, we already use the name like 'resumeUpload', which will provide the information to enduser.

@SammyK , your opinion?

This comment has been minimized.

@SammyK

SammyK Aug 20, 2015

Collaborator

I still like naming it a client since it's an HTTP client, but I'm not too fussy about the name. :)

This comment has been minimized.

@yguedidi

yguedidi Aug 20, 2015

Collaborator

@SammyK it is not an HTTP client, it's a video uploader, a helper service that ease resumable video uploading, and it has a dependency on an HTTP client, so I'm still for FacebookVideoUploader ;)

This comment has been minimized.

@SammyK

SammyK Aug 20, 2015

Collaborator

@yguedidi I can see your point. There are a number of features of the SDK that make HTTP calls and they are not always so obvious to the end-user that that's what happening in the background (like getAccessToken() for example). That's the main reason I suggested the name change, but again, I'm cool with keeping it as-is. :)

This comment has been minimized.

@codingtmd

codingtmd Aug 21, 2015

Let's just use FacebookVideoUploader.

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Aug 18, 2015

I did a quick scan a pointed out a few things. I think after @yguedidi has a look we should do some refactoring after it gets merged before we tag it as 5.1 :)

* @param int $target
* @param string $pathToFile
* @param string $accessToken
* @param int $maxTransferTries

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

Add some parameter descriptions here

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

this comment apply to all other public methods :)

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Aug 18, 2015

Also - can you add some docs? :)

while ($maxTransferTries > 0) {
try {
$res= $this->client->sendRequest($request)->getDecodedBody();

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

space

try {
$res= $this->client->sendRequest($request)->getDecodedBody();
$nextTransferChunk = new FacebookTransferChunk(

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

can be returned directly

This comment has been minimized.

@codingtmd

codingtmd Aug 19, 2015

True. $nextTransferChunk doesn't bring value here. Let's just return it directly in Line 145

return $nextTransferChunk;
} catch (FacebookSDKException $e) {
$preException = $e->getPrevious();
if ($preException instanceof FacebookResumableUploadException) {

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

please use an early return here

This comment has been minimized.

@codingtmd

codingtmd Aug 20, 2015

Not sure I get it. @yguedidi , are you suggesting to refactor the code like this?

                  $preException = $e->getPrevious();
                 if (! $preException instanceof FacebookResumableUploadException) {
                     throw $e;
                 }
                 if (--$maxTransferTries <= 0) {
                     $resumeContext = new FacebookResumeContext(
                          $endpoint,
                          $this->accessToken,
                          $chunk
                      );
                      $preException->setResumeContext($resumeContext);
                       throw $e;
                  }

This comment has been minimized.

@yguedidi

yguedidi Aug 20, 2015

Collaborator

Exactly! Early returns avoid nesting tabs too much

} else {
throw $e;
}
continue;

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

useless

{
$this->resumeContext = $resumeContext;
}

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

remove this extra empty line

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

BTW, I don't like the mutability of this class... A first solution that comes to me right now is to merge FacebookResumeContext in this exception class. @gfosco @SammyK thoughts?

This comment has been minimized.

@SammyK

SammyK Aug 19, 2015

Collaborator

I also don't like mutability. Not really crazy about merging FacebookResumeContext either. The FacebookResumeContext can be constructed by examining the request & response, right? This might be a bigger architecture issue. Can't really dive too deeply into the PR at the moment but I can make tweaks after a merge and before a tag to review.

This comment has been minimized.

@yguedidi

yguedidi Aug 19, 2015

Collaborator

As it's a little bit earlier in the night than yesterday, here a better idea: keep the context class, but slip to two exceptions, a FacebookFailedUploadException created in the FacebookResponseException, and the FacebookResumableUploadException instanciated with the context and throwed when needed, i.e. when we catch the failed exception in the transfer and want to allow an upload retry. :)
BTW, I still think we can have the three little properties of the context (without the max retries) as part of the FacebookResumableUploadException: the context and the exception will be instanciated at the same time.

This comment has been minimized.

@codingtmd

codingtmd Aug 21, 2015

Sounds a little overkilled. I feel merging FacebookResumeContext into FacebookResumableUploadException makes more sense, since this context is only used with resumableuploadexception. Could we just merge it?

This comment has been minimized.

@SammyK

SammyK Aug 21, 2015

Collaborator

Maybe merging is best if FacebookResumeContext is only ever used in the case of an exception.

use Facebook\Exceptions\FacebookSDKException;

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

should be removed

*
* @throws FacebookSDKException
*/
public function uploadVideo($target, $pathToFile, $accessToken, $maxTransferTries = 5)

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

You should add the Graph version

This comment has been minimized.

@SammyK

SammyK Aug 19, 2015

Collaborator

And falling back to the default Graph version:

$graphVersion = $graphVersion ?: $this->defaultGraphVersion;

This comment has been minimized.

@yguedidi

yguedidi Aug 19, 2015

Collaborator

Yep!

This comment has been minimized.

@codingtmd

codingtmd Aug 19, 2015

+1. For the chunked upload api, we only allow it with > v2.3. Here, the defaultGraphVersion should be v2.3.

This comment has been minimized.

@SammyK

SammyK Aug 20, 2015

Collaborator

The chunked video feature was added in 2.3 but I think it's back-versioned to work with all versions of Graph. I can't find the doc reference to that and I haven't tested it myself but I feel like I remember hearing that at F8 this year. :)

On that point though - I think we should allow these types of checks to happen on the Graph API side so we don't have to keep up with all these little "gotchas" on the SDK end as the Graph API changes. Thoughts?

This comment has been minimized.

@yguedidi

yguedidi Aug 20, 2015

Collaborator

👍 to let the Graph API handle it

This comment has been minimized.

@codingtmd

codingtmd Aug 21, 2015

Graph API has the check for versions. And it will handle the logic differently based on the api version. User can set whatever version they want, but here, for this function, since we use chunked upload, if we want a value for $this->defaultGraphVersion, it should be at least v2.3.

This comment has been minimized.

@SammyK

SammyK Aug 21, 2015

Collaborator

This won't be an issue since the fallback Graph version in the SDK will always be the latest version. :)

return [
'videoId' => $chunk->getVideoId(),
'success' => $uploader->finish($endpoint, $chunk->getUploadSessionId()),
];

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

above code starting from the do { can be extracted to a private function, as it's used in both uploadVideo and resumeUpload

This comment has been minimized.

@codingtmd
);
$res = $this->client->sendRequest($request)->getDecodedBody();
return $res['success'];

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

add an empty space above the return

$finishReqParams
);
$res = $this->client->sendRequest($request)->getDecodedBody();

This comment has been minimized.

@yguedidi

yguedidi Aug 18, 2015

Collaborator

@YuanhuiChen rename all your $res to $response, $decodedBody, $data, or any thing else but please, a full word. :)

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented Aug 19, 2015

@gfosco @SammyK I don't like merging "as is" and then refactor and/or fix things. I think when merging a PR, that PR should in a "ready to release" state. Just my 2 cents, will do how @gfosco want as usual :)

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Aug 19, 2015

@yguedidi Maybe we should send PR's to @YuanhuiChen's fork?

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented Aug 19, 2015

@SammyK it's a solution, but are we in a hurry? I think it's better to let @YuanhuiChen finish it, at least to learn more, as code review is an important part of the learning process :)
Of course, if @YuanhuiChen explicitly tell us he/she don't have the time to finish it, we'll help !

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Aug 19, 2015

@yguedidi No rush. Just thought we could submit some PR's to "talk code" instead of English as we try to figure out the best way to implement this feature since it's such a beast. :) But I'm OK with letting @YuanhuiChen do it without PR's from us as long as he has the time!

@yguedidi

This comment has been minimized.

Collaborator

yguedidi commented Aug 19, 2015

@SammyK sure I talk code better than english haha, but we also can't have some time, you know that too

@codingtmd

This comment has been minimized.

codingtmd commented Aug 20, 2015

Thank you, guys, especially for @YuanhuiChen to work on this task! I just left some comments in diff. We are pretty close, wow!

@gfosco

This comment has been minimized.

Contributor

gfosco commented Aug 22, 2015

I'm just the shepherd :) ... I am quite happy to see all the discussion and progress, and whenever you're all ready, we'll get 5.1 out. 👍

@gfosco

This comment has been minimized.

Contributor

gfosco commented Sep 3, 2015

@YuanhuiChen will you be able to make the requested changes? Thanks

@codingtmd

This comment has been minimized.

codingtmd commented Sep 4, 2015

@gfosco, I just heard yesterday that @YuanhuiChen already moved to another high priority task and she will not able to finish this diff. We are short of resource right now. Just wonder, is it possible to find some love from your side on this diff? Otherwise, we need to wait for another two weeks before we can find a new resource.

Cc @SammyK, @yguedidi

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Sep 4, 2015

I might have some time next week if we can't get anyone else on it. :)

@codingtmd

This comment has been minimized.

codingtmd commented Sep 6, 2015

Great! Thanks, @SammyK.

@codingtmd

This comment has been minimized.

codingtmd commented Sep 14, 2015

Hello @SammyK, how things going? :)

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Sep 14, 2015

Hey @codingtmd! Thanks for the ping! I didn't get a chance to look at this last week so I'm going to see what's involved with this tomorrow and let you know. :)

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Sep 16, 2015

Hey @YuanhuiChen! If I send a PR to your branch for this feature, will you be able to merge any changes so we can continue the discussion here? :)

@codingtmd

This comment has been minimized.

codingtmd commented Sep 16, 2015

Hello @SammyK, @YuanhuiChen already left this project, so I'm not sure whether she will response or not. Is it possible to send a PR on the master?:)

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Sep 21, 2015

I think I'll try to create a new branch from @YuanhuiChen's fork so that all the existing commits will stay in place. Things got busy all of a sudden on my end but I'm still trying to work on this when I can. :)

@SammyK SammyK referenced this pull request Sep 22, 2015

Merged

Video upload by chunks #498

@SammyK

This comment has been minimized.

Collaborator

SammyK commented Sep 22, 2015

Moved this PR to #498 so we can close this one. :)

@gfosco gfosco closed this Sep 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment