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

Fix issue #354 #355

Merged
1 commit merged into from
Mar 10, 2017
Merged

Fix issue #354 #355

1 commit merged into from
Mar 10, 2017

Conversation

groundflyer
Copy link
Contributor

This should fix #354

@ghost
Copy link

ghost commented Feb 20, 2017

Ignore this comment, I posted the wrong code in the wrong issue.. sorry!
Fellow developers.. I see no difference in the video output when this PR is applied.
here is the code I'm using..

_clip = VideoFileClip("media/knights.mp4")
_clip1=CompositeVideoClip([_clip.set_duration(20)]).crossfadeout(1)
_clip1.write_videofile("test.mp4", codec="mpeg4")

Am I overlooking something? Can someone else replicate my results?

@keikoro
Copy link
Collaborator

keikoro commented Feb 25, 2017

@Earney I haven't tested this, but are you saying you're still getting the error originally described by @groundflyer in #354?

@ghost
Copy link

ghost commented Feb 25, 2017

I'm not sure if the fix does anything. I do not see a difference in the output file "test.mp4". I thought I'd let @Zulko take a look. Maybe I'm overlooking something.

@keikoro
Copy link
Collaborator

keikoro commented Feb 25, 2017

@Earney I think the commit wasn't supposed to change the output file, but to get rid of an error. If the error can't be reproduced with the current code (minus the fix), maybe the issue is not valid anymore.

@ghost
Copy link

ghost commented Mar 1, 2017

My bad guys.. I posted the wrong set of code here about 10 days ago. The pull request does solve the issue for the problem in #354 . should we look at merging this?

@ghost
Copy link

ghost commented Mar 1, 2017

Or is the @required_duration for the function crossfadeout unnecessary? crossfadein does not have a @required_duration ?

@ghost
Copy link

ghost commented Mar 1, 2017

the @required_duration on crossfadeout isn't necessary. The duration value error is checked further down in the fadeout function. But I believe this issue, brings up an issue with that the mask.duration is not being set somewhere. There are other issues on this board that bring this up, and I fill that this pull request addresses the issue, and not the real problem (ie, the mask.duration is not being set when it should).

@keikoro
Copy link
Collaborator

keikoro commented Mar 1, 2017

@Earney At first I thought you were referring to someone because of the @ sign.

Would you consider using backticks for code bits so they can be better differentiated from normal text? I know it's a bit more work to type messages that way but it improves their readability a lot.

@ghost
Copy link

ghost commented Mar 1, 2017

so you mean @required_duration vs @required_duration ?

@keikoro
Copy link
Collaborator

keikoro commented Mar 1, 2017

@Earney Yes!! (:

@ghost ghost self-assigned this Mar 1, 2017
@ghost
Copy link

ghost commented Mar 2, 2017

I've finally figured out what is going on.

Because TextClip inherits from ImageClip, and by default transparent = true, ImageClip will create a mask at class instantiation. Since the duration is not known at this time, the duration is set to none. There are a few solutions. One, when someone sets the duration of the clip, we could also set the duration of the mask. but if the mask gets updated (or replaced) we would need to reset the duration as well. The second option is to check that the masked duration is correct before we use it, which is similar to what this PR is doing. What method do people recommend?

@ghost
Copy link

ghost commented Mar 7, 2017

I'm currently leaning toward doing what this pull request does. In each fx, setting the mask.duration = c.duration. That way, the mask duration is guaranteed to match the clips duration. if there is no objection, in the next week or so, I'll start working on this.

@ghost ghost merged commit 484ea99 into Zulko:master Mar 10, 2017
This pull request was closed.
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.

crossfadeout "Attribute 'duration' not set"
2 participants