Fix Writing Transparent Video Files

[stratty7]

This will allow for clips that are loaded with an alpha channel (a .mov file for example) to be writen back out as a .mov file with an alpha layer (transparent background). The code already largely exists for this, the write_videofile function simply needs to be allowed to pass the with_mask arg to the ffmpeg_write_video, which then handles the rest.

For example:

transparentclip = VideoFileClip("1.mov", has_mask=True)
transparentclip.write_videofile("test.mov", codec="png", fps=30, with_mask=True)

Longer example:

transparentclip = VideoFileClip("1.mov", has_mask=True)
transparentclip.write_videofile("test.mov", codec="png", fps=30, with_mask=True)

background = VideoFileClip("background.mp4")
overlay = VideoFileClip("test.mov", has_mask=True)
comp = CompositeVideoClip([background, overlay])
comp.write_videofile("final.mp4", fps=30, codec="libx264")

Even longer

# Where 1.mov and 2.mov both have an alpha channel
clips = [VideoFileClip("1.mov", has_mask=True), VideoFileClip("2.mov", has_mask=True)]
concat = concatenate_videoclips(clips, method="chain")
concat.write_videofile("test.mov", codec="png", fps=30, with_mask=True)
background = VideoFileClip("background.mp4")
overlay = VideoFileClip("test.mov", has_mask=True)
comp = CompositeVideoClip([background, overlay])
comp.write_videofile("final.mp4", fps=30, codec="libx264")

• I have provided code that clearly demonstrates the bug and that only works correctly when applying this fix
• I have properly documented new or changed features in the documentation or in the docstrings

[coveralls]

coverage: 81.904%. remained the same
when pulling 58cb92e on stratty7:fix/add-with-mask-to-write-video-file
into bc8d1a8 on Zulko:master.

[keikoro]

@stratty7 Looks like there are problems with formatting which would need fixing.

Also, looking at changes that came before this addition: shouldn't this be propagated to elsewhere as well? E.g. when pixel_format was introduced, it got added in more than one place.

[keikoro]

Hmm, looking at the above examples as well as other files, I guess I'm confused about two things:

1. What's the difference between has_mask and with_mask?
2. What's the difference between your addition and this instance of with_mask?

When I mentioned pixel_format above, which is a line above your change, I was specifically talking about that it got added to VideoClip.py and VideoFileClip.py simultaneously (as well as other places) in #1237 (where it was still called pix_fmt). Your change is only for the former. Looking at the latter, I can see that has_mask originates there. Do they set the same thing?

[stratty7]

Hmm, looking at the above examples as well as other files, I guess I'm confused about two things:

1. What's the difference between has_mask and with_mask?
2. What's the difference between your addition and this instance of with_mask?

When I mentioned pixel_format above, which is a line above your change, I was specifically talking about that it got added to VideoClip.py and VideoFileClip.py simultaneously (as well as other places) in #1237 (where it was still called pix_fmt). Your change is only for the former. Looking at the latter, I can see that has_mask originates there. Do they set the same thing?

@keikoro I understand your points and see what you mean, I think.

With mask is not used in the ffmpeg writer at all, and using the pixel_format can achieve the same result without the need of adding any more code. I have changed the code to reflect that and removed the has_mask arg from ffmpeg_writer.

transparentclip = VideoFileClip("1.mov", has_mask=True)
transparentclip.write_videofile("test.mov", codec="png", fps=30, pixel_format="rgba")

[stratty7]

I think using pixel_format="rgba" addresses my needs.
However, with_mask is currently not passed to ffmpeg_write_video anywhere, I think this could probably be cleaned up. I will close this PR and if I get time clean that up and open a new one. Thanks for the reviews.

[keikoro]

To clarify my last comment: I only used pixel_format as an example for a change that was done in both files, VideoClip.py and VideoFileClip.py, so thought that maybe your suggested change should also happen in both files. Which then led me to the with_mask vs. has_mask thing.

I didn't look at it in more detail, but am I understanding correctly that this discrepancy was actually introduced earlier, and pixel_format was potentially, partially (?) included to address the issue of one argument not getting passed down the chain? Or were you more saying that you just had the realisation pixel_format could be used to replace the other argument, even though it wasn't so far?

[stratty7]

Or were you more saying that you just had the realisation pixel_format could be used to replace the other argument, even though it wasn't so far?

Yeah this.

I think pixel_format could be used in the place of with_mask at the moment. Only if with_mask is implemented properly into write_videofile and ffmpeg_write_video (which it currently isn't, but would be nice to have) then I understand having both args, but currently having both seems somewhat pointless. I think implementing with_mask properly and fully (instead of my bandaid fix in the original code of this pr) into write_videofile and ffmpeg_write_video is the true fix here, but @convert_masks_to_RGB may be a large obstacle to overcome..

That is why I have closed this for now and will work on it when i find time.