Fix crashes and failures on decoding some types of album art images #390

PhysSong · 2019-12-17T02:33:43Z

Add a function to check if there's an album art, using the AV_DISPOSITION_ATTACHED_PIC flag.
Check for nullity of last_video_frame before dereferencing it. Previously, it was wrongly checked by last_frame. It made OpenShot crash when it failed to decode the last frame.
Specify AV_CODEC_FLAG_LOW_DELAY flag for album arts. FFmpeg's PNG decoder and TIFF decoder support per-frame threading, and it's turned on by default. However, the feature adds some latency and breaks album art decoding.

I removed checked_frames[requested_frame] > 8 because removing it didn't break anything, but please let me know if this one is still needed.

ferdnyc

Thanks @PhysSong , it'd be great to get this issue cleared up.

This is an incomplete, kind of drive-by first impressions non-review, I'll try to give it another look later. There is one significant issue that I noticed, and I think the code needs to at least be tested in this context (if it hasn't been already): Video files with embedded thumbnail images. (Which are actually a thing.)

ferdnyc · 2019-12-17T19:03:21Z

src/FFmpegReader.cpp

@@ -638,6 +650,11 @@ void FFmpegReader::Close() {
 	}
 }

+bool FFmpegReader::HasAlbumArt() {
+	return pFormatCtx && videoStream >= 0 && pFormatCtx->streams[videoStream]
+		&& (pFormatCtx->streams[videoStream]->disposition & AV_DISPOSITION_ATTACHED_PIC);


AV_DISPOSITION_ATTACHED_PIC has existed since FFmpeg 1.0 or earlier, so no issues with the #define as long as pFormatCtx->streams[]->disposition is valid and has the same semantics for all FFmpeg 2.x ... 4.2 (target supported versions). 👍

Will this do the right thing for video files with attached cover art, BTW? For instance, MP4 videos (and probably MKV as well) can have thumbnail images attached that are shown in the file browser in lieu of a system-generated still frame image. This won't break on those files, will it?

I looked at the FFmpeg codebase. It seems like some formats such as .mov, mkv, wtv, asf may contain streams with AV_DISPOSITION_ATTACHED_PIC flag, but I think I need to test these cases.

system-generated still frame image

That seems to irrelevant, though.

system-generated still frame image

That seems to irrelevant, though.

Oh, yeah, sorry, it is. To this PR, I mean. System thumbnails are managed external to the media files.

@PhysSong

If you need a test file, you can use mkvpropedit to add a PNG attachment to an MKV file:

mkvpropedit Video.mkv --add-attachment Image.png

(mkvpropedit is part of the MKVToolNix suite.)

I tested this with the step you provided. Since libopenshot takes the first video stream, HasAlbumArt() will check if that one is a static image. ffprobe says the video cover art is the second video stream, meaning that libopenshot won't recognize the image as album art.

I'll add a comment to explain how this function works.

ferdnyc · 2019-12-17T19:26:24Z

src/FFmpegReader.cpp

+				// Disable per-frame threading for album arts
+				// Using FF_THREAD_FRAME adds one frame decoding delay per thread,
+				// but there's only one frame in this case.
+				if (HasAlbumArt())
+				{
+#ifdef AV_CODEC_FLAG_LOW_DELAY
+					pCodecCtx->flags |= AV_CODEC_FLAG_LOW_DELAY;


(Just a curiosity / background question, I'm not suggesting this isn't the right implementation or that it needs to be changed in any way:)

Can you give us some detail on what led you to setting low-delay as an AVCodecContext flag, to address this? Are there other options? (Things like modifying thread_count directly, setting low_delay or has_b_frames some other way, etc...) Are there problems with those options that make AV_CODEC_FLAG_LOW_DELAY a better choice?

modifying thread_count

I haven't tested it yet, but I think that will work.

setting low_delay or has_b_frames

Some decoders may ignore them, so I think it's better not using them.

FYI, as an alternative, I can use thread_type to achieve the same goal.

ferdnyc · 2019-12-18T01:18:51Z

src/FFmpegReader.cpp

-		// Seek video stream (if any)
-		if (!seek_worked && info.has_video) {
+		// Seek video stream (if any), except album arts
+		if (!seek_worked && info.has_video && !HasAlbumArt()) {


I'm concerned the same issue may bite us here: MP4 and MKV video files can have attached picture data.

ferdnyc · 2019-12-18T01:19:50Z

src/FFmpegReader.cpp

-		if (checked_frames[requested_frame] > 8 && !missing_video_frames.count(requested_frame) &&
+		if (!missing_video_frames.count(requested_frame) &&
 			!processing_audio_frames.count(requested_frame) && processed_audio_frames.count(requested_frame) &&
-			last_frame && last_video_frame->has_image_data && aCodecId == AV_CODEC_ID_MP3 && (vCodecId == AV_CODEC_ID_MJPEGB || vCodecId == AV_CODEC_ID_MJPEG)) {
+			last_video_frame && last_video_frame->has_image_data && HasAlbumArt()) {


Same issue here as well, by removing the AV_CODEC_ID_MP3 and AV_CODEC_ID_MJPEG[B] conditions.

That's intentional. I want to make the logic work for other file types or image types.

@PhysSong Understood, and it definitely needs to be removed if album art is going to be supported across audio files generally. (Most of which, these days, are not MP3s with JPEG-encoded image payloads.)

As long as HasAlbumArt() works for both audio and video files (returning the answer we want, in all the various input file cases), then I'm on board with removing as many of the other conditions as possible.

ferdnyc · 2019-12-18T01:43:26Z

IMHO the (existing) code for dealing with album art may actually be too abstract, trying to shoe-horn it into the video processing logic. It would probably be easier, in all honesty, to just make use of the info.has_single_image state — which exists in ReaderBase, but which FFmpegReader never even looks at — for files where we want to display album art instead of video content (present or not).

Then, when info.has_single_image is true, all of the video-decoding logic can just be bypassed entirely, in favor of simply returning frames of the still image the same way generative image readers like QtImageReader do.

(We should also take care to only set has_single_image on FFmpegReader files where we WANT to use the embedded still image instead of any video stream that may or may not be present. Either that, or we add a separate switch to control that part of the logic, maybe use_single_image or ignore_video. The (purely semantic) difference being that the former would depend on has_single_image being true in order to have any effect, the latter wouldn't.)

PhysSong · 2019-12-18T23:11:35Z

@ferdnyc Thanks for the review! I'll test this with more files and update the PR this weekend.

ferdnyc · 2019-12-19T02:24:57Z

src/FFmpegReader.cpp

 		// If MP3 with single video frame, handle this special case by copying the previously
 		// decoded image to the new frame. Otherwise, it will spend a huge amount of
 		// CPU time looking for missing images for all the audio-only frames.
-		if (checked_frames[requested_frame] > 8 && !missing_video_frames.count(requested_frame) &&
+		if (!missing_video_frames.count(requested_frame) &&
 			!processing_audio_frames.count(requested_frame) && processed_audio_frames.count(requested_frame) &&
-			last_frame && last_video_frame->has_image_data && aCodecId == AV_CODEC_ID_MP3 && (vCodecId == AV_CODEC_ID_MJPEGB || vCodecId == AV_CODEC_ID_MJPEG)) {
+			last_video_frame && last_video_frame->has_image_data && HasAlbumArt()) {
 			missing_video_frames.insert(std::pair<int64_t, int64_t>(requested_frame, last_video_frame->number));
 			missing_video_frames_source.insert(std::pair<int64_t, int64_t>(last_video_frame->number, requested_frame));
 			missing_frames.Add(last_video_frame);


This bit here... this is what I meant about how the existing logic (meaning the code prior to this PR) is — IMHO — too convoluted. It overcomplicates things for itself.

(I didn't write this code, these are my impressions of what it does and the intent behind it. I could be mistaken about some or all of it, so if I say anything that appears to be at odds with reality then I'd suggest trusting reality and assuming I'm just wrong.)

Because it treats the cover art as the video stream for an audio file, and attempts to process it exactly the same as any other video, it does a lot of stuff to "engineer" the right conditions where that can happen:

last_frame probably was intentional, as a too-efficient test that would be better written explicitly as last_frame > 0. The intent being to avoid this copying "bypass" logic when processing the first frame — that's the one you're going to need to copy to all the others. last_frame gets explicitly reset to 0 whenever the processing state is reset.

(That being said, checking non-nullity of last_video_frame is probably sufficient, and makes it unnecessary to test last_frame. So, I think your change there probably still makes sense. My point is just that I don't think last_frame was a mistake, just a poorly-expressed way to test a certain condition.)

checked_frames[requested frame] > 8 is part of the whole video/still-image conflation.

If you do have a file with a video stream, you want that to take priority over the album art image, so requiring cache misses before performing this forced copy helps ensure that you won't ignore the actual video stream in favor of the attachment.

BUUUUT, because frames can be processed both in parallel and out of order, you also can't be sure whether a requested_frame exists but is being processed in another thread, will exist but is simply late, or won't exist because the file doesn't have a video stream to pull it from. So, requiring checked_frames[requested_frame] > 8 is (I'm guessing) a way to just say, "Hold off on all of those decisions until we've processed a bit more of the file, so we can make better assumptions."

It's incredibly nebulous and weird, which again IMHO makes it not a great approach — but most likely every part of that (crazy) conditional was added on as a band-aid to avert some issue found in testing, which means that every part of it probably is "necessary" in at least some (possibly obscure) instances.

Though it would certainly be better to replace the whole mess with a correct implementation, meaning one that's deterministic enough to not rely on assumptions or require extensive band-aids before it will do the right thing for all cases.

Okay, thanks for the explanation. I'll look into those possibilities and make a decision.

I surveyed the lay of the land w/r/t cover art support. Currently, this is how OpenShot 2.4.4 with libopenshot 0.2.3 handles the file metadata when you load...

an MP3, OGG, or M4A audio file without cover art:

info.has_audio: true

info.has_video, info.has_single_image: false

info.vcodec: ""

an MP3 file with JPG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "mjpeg"

it shows a thumbnail, and shows the image when previewing the file

an MP3 file with PNG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "png"

it does not show a thumbnail of the image

it hangs trying to preview the file, eventually crashing after spewing dozens of "error while seeking video stream" messages

an Ogg Vorbis audio file with JPG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "mjpeg"

it shows a thumbnail of the image

it crashes instantly trying to preview the file: "Assertion stream_index < ogg->nstreams failed at libavformat/oggdec.c:925"

an Ogg Vorbis audio file with PNG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "png"

it does not show a thumbnail of the image

it hangs briefly trying to preview the file, then crashes: "Assertion stream_index < ogg->nstreams failed at libavformat/oggdec.c:925"

an M4A audio file with JPG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "mjpeg"

it shows a thumbnail, and shows the image when previewing the file

an M4A audio file with PNG cover art:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "png"

it does not show a thumbnail of the image

it hangs for several seconds trying to preview the file, spewing "error while seeking video stream" messages, but eventually abandons attempts to display the image and returns to normal function

An MP4 video file with or without image attachment, any format:

info.has_audio, info.has_video: true

info.has_single_image: false

info.vcodec: "h264"

it shows a frame-capture thumbnail

preview playback displays the video stream

So, that's pretty consistent with the special-case file handling being restricted to aCodecId == AV_CODEC_ID_MP3 && (vCodecId == AV_CODEC_ID_MJPEGB || vCodecId == AV_CODEC_ID_MJPEG)).

(Except that I'm surprised case 6 works, the M4A file with JPG cover art. I don't really have an explanation for that one. I just double-checked and my test file was AAC encoded, not MP3, yet it still works. However, playback is terrible, with tons of stuttering and a lot of complaining from FFmpeg on the console output. So, I have a feeling that case is ending up in that "Otherwise, it will spend a huge amount of CPU time looking for missing images for all the audio-only frames." state mentioned in the comments.)

If I had the energy, I'd test an MP4 video file with an MJPEG-encoded video stream and an attached image (especially if it also contained MP3-encoded audio), but I'd have to make one in order to test it and TBH that's an esoteric enough format that I've decided I really don't care what the results would be. So, meh.

The more I'm looking at this, the more I think it would make sense to export cover art when available by leveraging the existing info.has_single_image property — whether the file is video or audio, regardless what codecs it uses. But that would involve probably adding an additional field for the encoding of the image data (rather than overloading info.vcodec which I've decided was a terrible idea), it would involve changes to the exported user interface to add an "Enable Image" keyframed property (to supplement "Enable Video" and "Enable Audio"), it would involve backend changes to handle the image "stream" differently from the video stream (but still present it to the writer as video frames), and most importantly we're talking deep architectural stuff that's not at all what you signed up for here.

Nearer-term, if you can get OpenShot to handle cover art in non-JPEG and/or non-MP3 audio files without hanging, crashing, or playing like it's half-comatose, and without breaking support for video files that happen to also include attached images, then I think that's way more than anyone could expect and you'd be making a major improvement to OpenShot's audio-file handling. Anything you wanted to do beyond that to improve the situation would be above and beyond, though certainly very much appreciated.

Thank you for the kind information again! I'm planning to do more tests that I missed initially. 🙂

@PhysSong Great, thanks. In the meantime, I'm just going to retitle this PR to indicate that it's not yet ready for merge. Feel free to remove the "WIP" when it's updated.

checked_frames[requested frame] > 8 is part of the whole video/still-image conflation.

I see. Probably I need to think more about it. 🙂

PhysSong · 2020-01-02T06:20:17Z

src/FFmpegReader.cpp

+		if (HasAlbumArt()) {
+			ret = avcodec_send_packet(pCodecCtx, &(pFormatCtx->streams[videoStream]->attached_pic));
+		}
+		else {
+			ret = avcodec_send_packet(pCodecCtx, packet);
+		}


Decoding works both with and without this code, but I need to test if this brings any performance changes.

jonoomph · 2020-01-02T20:46:48Z

@PhysSong This is looking great! Thanks for the help, and thanks @ferdnyc for helping as well. Is this close to being ready for a merge? Or does it still need quite a bit of refinement and testing?

PhysSong · 2020-01-21T06:51:04Z

Sorry for the inactivity. I'll update this PR in a few days.

PhysSong · 2020-01-30T05:47:00Z

I have some problems testing this PR due to OpenShot/openshot-qt#3043 (comment) which seems to be somehow related to the seeking system. This PR fixes the additional crash, but OpenShot still hangs.

SuslikV · 2020-01-30T07:54:50Z

@PhysSong to workaround the seeking issues try to make long clip and seek at the end of it (thus libopenshot wouldn't reopen the file and number of crashes will reduce).

PhysSong · 2020-05-29T02:15:54Z

I'm back here now. 😃 I need to look at upstream changes first, and then rework this.

codecov · 2020-05-29T03:17:42Z

Codecov Report

Merging #390 into develop will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff            @@
##           develop     #390   +/-   ##
========================================
  Coverage    49.45%   49.46%           
========================================
  Files          129      129           
  Lines        10261    10264    +3     
========================================
+ Hits          5075     5077    +2     
- Misses        5186     5187    +1

Impacted Files	Coverage Δ
src/FFmpegReader.h	`33.33% <ø> (ø)`
src/FFmpegReader.cpp	`68.70% <85.71%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07a447c...c0db260. Read the comment docs.

PhysSong · 2020-10-20T02:02:59Z

Now it's ready for reviews.

jonoomph · 2021-01-27T03:44:41Z

Thanks @PhysSong and @ferdnyc. I've been testing this out tonight, and everything seems to work great. I'm merging. Thanks again and sorry for how long this took to get merged. Much appreciated.

PhysSong · 2021-01-28T01:41:57Z

Great to see this one merged, thanks!

PhysSong force-pushed the albumartfixes branch from eca613e to eab0865 Compare December 17, 2019 02:36

ferdnyc reviewed Dec 18, 2019

View reviewed changes

ferdnyc reviewed Dec 19, 2019

View reviewed changes

ferdnyc changed the title ~~Fix crashes and failures on decoding some types of album art images~~ WIP: Fix crashes and failures on decoding some types of album art images Dec 25, 2019

PhysSong commented Jan 2, 2020

View reviewed changes

Fix crashes and failures on decoding some types of album art images

d338e0b

PhysSong force-pushed the albumartfixes branch from eab0865 to d338e0b Compare May 29, 2020 03:10

ferdnyc and others added 2 commits October 19, 2020 16:40

Merge branch 'develop' into albumartfixes

04bcab4

Add a comment explaining FFmpegReader::HasAlbumArt()'s behavior

c0db260

PhysSong changed the title ~~WIP: Fix crashes and failures on decoding some types of album art images~~ Fix crashes and failures on decoding some types of album art images Oct 20, 2020

PhysSong requested a review from ferdnyc November 12, 2020 03:29

jonoomph merged commit ec31234 into OpenShot:develop Jan 27, 2021

PhysSong deleted the albumartfixes branch January 28, 2021 01:41

jkirk mentioned this pull request Dec 14, 2021

(Some) MP3 files containing cover art cause OpenShot to crash on Preview/Export OpenShot/openshot-qt#3043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crashes and failures on decoding some types of album art images #390

Fix crashes and failures on decoding some types of album art images #390

PhysSong commented Dec 17, 2019

ferdnyc left a comment

ferdnyc Dec 17, 2019

ferdnyc Dec 17, 2019

PhysSong Dec 18, 2019

ferdnyc Dec 19, 2019

ferdnyc Dec 19, 2019 •

edited

PhysSong Dec 26, 2019

PhysSong Jan 2, 2020

ferdnyc Dec 17, 2019

PhysSong Dec 26, 2019

ferdnyc Dec 18, 2019

ferdnyc Dec 18, 2019 •

edited

PhysSong Dec 26, 2019

ferdnyc Jan 6, 2020

ferdnyc commented Dec 18, 2019 •

edited

PhysSong commented Dec 18, 2019

ferdnyc Dec 19, 2019 •

edited

PhysSong Dec 20, 2019

ferdnyc Dec 20, 2019

PhysSong Dec 22, 2019

ferdnyc Dec 25, 2019

PhysSong Dec 26, 2019

PhysSong Jan 2, 2020

jonoomph commented Jan 2, 2020

PhysSong commented Jan 21, 2020

PhysSong commented Jan 30, 2020

SuslikV commented Jan 30, 2020

PhysSong commented May 29, 2020

codecov bot commented May 29, 2020 •

edited

PhysSong commented Oct 20, 2020

jonoomph commented Jan 27, 2021

PhysSong commented Jan 28, 2021

Fix crashes and failures on decoding some types of album art images #390

Fix crashes and failures on decoding some types of album art images #390

Conversation

PhysSong commented Dec 17, 2019

ferdnyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdnyc Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdnyc Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferdnyc commented Dec 18, 2019 • edited

PhysSong commented Dec 18, 2019

ferdnyc Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonoomph commented Jan 2, 2020

PhysSong commented Jan 21, 2020

PhysSong commented Jan 30, 2020

SuslikV commented Jan 30, 2020

PhysSong commented May 29, 2020

codecov bot commented May 29, 2020 • edited

Codecov Report

PhysSong commented Oct 20, 2020

jonoomph commented Jan 27, 2021

PhysSong commented Jan 28, 2021

ferdnyc Dec 19, 2019 •

edited

ferdnyc Dec 18, 2019 •

edited

ferdnyc commented Dec 18, 2019 •

edited

ferdnyc Dec 19, 2019 •

edited

codecov bot commented May 29, 2020 •

edited