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.

Sign up for GitHub

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

Jump to bottom

Fix videos with alpha channel #609

Merged
merged 1 commit into from
Jan 1, 2021
Merged

Fix videos with alpha channel #609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/FFmpegReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,7 +1352,13 @@ void FFmpegReader::ProcessVideoPacket(int64_t requested_frame) {
std::shared_ptr<Frame> f = CreateFrame(current_frame);

// Add Image data to frame
f->AddImage(width, height, 4, QImage::Format_RGBA8888_Premultiplied, buffer);
if (!ffmpeg_has_alpha(AV_GET_CODEC_PIXEL_FORMAT(pStream, pCodecCtx))) {
// Add image with no alpha channel, Speed optimization
f->AddImage(width, height, 4, QImage::Format_RGBA8888_Premultiplied, buffer);
} else {
// Add image with alpha channel (this will be converted to premultipled when needed, but is slower)
f->AddImage(width, height, 4, QImage::Format_RGBA8888, buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. It'd be good to get a test file with alpha channel, if possible, to unit-test this. Presumably someone must have one, if they noticed this in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've recently learned that there are many formats which can allow an alpha channel, and thus, we can always add a transparent video into our unittests (I like that idea), but it won't test for all possible alpha formats. So, it still leaves the possibility that a format is not working correctly, but I suppose that will just be the case.

Copy link
Contributor

Choose a reason for hiding this comment

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

FFmpeg does actually have a premultiply filter (accessible from code as avfilter_get_by_name("premultiply");), which in theory could be imposed into the processing graph to have it produce premultiplied alpha values. But I have a feeling we're better off letting Qt handle that.

}

// Update working cache
working_cache.Add(f);
Expand Down
10 changes: 10 additions & 0 deletions src/FFmpegUtilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,16 @@
#define PIX_FMT_YUV444P AV_PIX_FMT_YUV444P
#endif

// Does ffmpeg pixel format contain an alpha channel?
inline static const bool ffmpeg_has_alpha(PixelFormat pix_fmt)
{
if (pix_fmt == AV_PIX_FMT_ARGB || pix_fmt == AV_PIX_FMT_RGBA || pix_fmt == AV_PIX_FMT_ABGR || pix_fmt == AV_PIX_FMT_BGRA || pix_fmt == AV_PIX_FMT_YUVA420P) {
return true;
} else {
return false;
}
}
Comment on lines +130 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

	inline static const bool ffmpeg_has_alpha(PixelFormat pix_fmt)
	{
		if (pix_fmt == AV_PIX_FMT_ARGB || pix_fmt == AV_PIX_FMT_RGBA || pix_fmt == AV_PIX_FMT_ABGR || pix_fmt == AV_PIX_FMT_BGRA || pix_fmt == AV_PIX_FMT_YUVA420P) {
			return true;
		} else {
			return false;
		}
	}

For maintainability this might be better as,

        inline static const bool ffmpeg_has_alpha(PixelFormat pix_fmt) {
            const AVPixFmtDescriptor *fmt_desc = av_pix_fmt_desc_get(pix_fmt);
            return bool(desc->flags & AV_PIX_FMT_FLAG_ALPHA);
        }

That way it'll support any new pixel formats they add in the future, too. Near as I can tell it'd be compatible with every FFmpeg back to at least 2.4, which is already way farther back than we should worry about going.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the static storage specifier on that may have to come off, if it's going to be taking struct-pointer return values from av_pix_fmt_desc_get() and dereferencing them. But an inline (non-static) function should work fine, I think?

Otherwise, it can always just be added to FFmpegReader.cpp.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonoomph See my previous comment(s), regarding your email question.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonoomph Oops. Just realized this had a typo in it, it should've been

        inline static const bool ffmpeg_has_alpha(PixelFormat pix_fmt) {
            const AVPixFmtDescriptor *fmt_desc = av_pix_fmt_desc_get(pix_fmt);
            return bool(fmt_desc->flags & AV_PIX_FMT_FLAG_ALPHA);
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks terrific. I'm gonna test it right now! This is exactly what I was searching for, lol. I have a few transparent videos I've been using to test this, so it should be a quick check.


// FFmpeg's libavutil/common.h defines an RSHIFT incompatible with Ruby's
// definition in ruby/config.h, so we move it to FF_RSHIFT
#ifdef RSHIFT
Expand Down