Fix videos with alpha channel #609
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #609 +/- ##
========================================
Coverage 52.31% 52.32%
========================================
Files 129 129
Lines 10783 10789 +6
========================================
+ Hits 5641 5645 +4
- Misses 5142 5144 +2
Continue to review full report at Codecov.
|
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@jonoomph See my previous comment(s), regarding your email question.
There was a problem hiding this comment.
@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);
}There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
This is a regression from using premultiplied colorspace in Qt. If a video has an alpha channel, we need to convert the RGBA correctly to premultiplied (which is slower), but without the conversion, all the alpha values are wildly wrong.