[iOS]: prevent crashing (EXC_BAD_ACCESS) when releasing FFMPEG contex…

[yoshisuga]

…t by adding NULL check on codec context pointer

Ran into regular crashes when running Final Fantasy VII: Crisis Core (J) on iOS. Tracked the crashes down to this block of code. The codec context pointer was NULL so...yeah :)

[unknownbrackets]

Indent with hard tabs, please. If the diff on GitHub looks wrong, the indentation is incorrect.

-[Unknown]

[unknownbrackets]

Is this still an issue with the latest version of FFmpeg? It was updated in 84216ba recently.

The FFmpeg ABI must've changed. Before we were leaking in the new version of FFmpeg, the one we've been using for over a year on all other platforms than iOS.

-[Unknown]

[hrydgard]

We don't put spaces inside "if parenthesis" either, so make it:

if (pCodecCtx != NULL)

In addition, please add an "else" clause that ERROR_LOGs something like "Unexpected: pCodecCtx is NULL".

[yoshisuga]

I believe this is still an issue with the latest ffmpeg, but i'm not 100% sure. It looks like the ios build uses a precompiled version of ffmpeg and i haven't been successful in building it myself. I was on the latest master and would still catch the condition where the codec context pointer is null.

[unknownbrackets]

Sorry to keep nitpicking. There's more indentation issues here. I'm taking a harder look now, and I wonder if the simpler solution isn't just:

        // Future versions may add other things to free, but avcodec_free_context didn't exist yet here.
-       avcodec_close(pCodecCtx);
-       av_freep(&pCodecCtx->extradata);
-       av_freep(&pCodecCtx->subtitle_header);
-       av_freep(&pCodecCtx);
+       av_freep(&pCodecCtx->extradata);
+       av_freep(&pCodecCtx->subtitle_header);
+       avcodec_close(pCodecCtx);
+       av_freep(&pCodecCtx);

That is, freeing extradata and subtitle_header before pCodecCtx. They are leaking otherwise, per the FFmpeg documentation.

Per the docs:

Close a given AVCodecContext and free all the data associated with it (but not the AVCodecContext itself).

If you look at the source, you'll note it doesn't always free these two pointers. They added avcodec_free_context to resolve this common mistake in API usage.

I'm guessing the older FFmpeg must've freed the AVCodecContext itself in avcodec_close? Or something?

What do you think? Does the above change work for you?

Anyway, you should be getting the new code now, the one inside #if LIBAVCODEC_VERSION_INT >= AV_VERSION_INT(55, 52, 0). So if the code you're changing even matters on iOS, it still means the you're using an outdated, broken FFmpeg. This version of FFmpeg WILL have bugs in other places and playing some music from some games anyway. There are important bugfixes in FFmpeg that we need in the newer version.

That doesn't mean your change is bad or anything, because some platforms still use this old FFmpeg. Just trying to explain. And thanks for identifying why this was crashing - I'd gotten the idea it was in this area, but didn't know the cause.

Yeah, ffmpeg for iOS is a pain to compile, and requires gas-preprocessor.pl. I had to change ios-build as well to make it accept the latest Xcode, but I'm not sure my changes were correct. I'm thinking to update the instructions but I couldn't get it to work in a clean env a second time, so I'm not sure of the correct steps.

This is probably useful:
https://github.com/chrisballinger/FFmpeg-iOS

Anyway, I compiled the latest version only very recently. It fixed a crash for someone using a clean build and updated submodules. So if you run git submodule update --recursive, you should get the new precompiled version.

-[Unknown]

[yoshisuga]

I'm not even remotely familiar with ffmpeg, but I appreciate the explanation :) In fact, I'm totally unfamiliar with the ppsspp codebase and I really just bandaided this fix in to avoid the crashing. Please don't apologize for nitpicking, etc, it's just part of the pull process and I'm totally fine with any feedback. I'm coming from a place of total ignorance so I really appreciate any comments :)

I'll test out your suggested change, but my gut feeling tells me its going to crash because I believe it's happening when trying to access the "extradata" attribute - the pCodecCtx pointer already doesn't point anywhere and i think it's going to crash when it tries to access that. I could be wrong, though - but i will test your change out - thanks!

[yoshisuga]

Ok, I was completely wrong about my comments about it going to crash earlier - it looks like your fix is working! I'm not sure but I think the video playback seems smoother as well!

I'll re-commit my change shortly! Thanks!!

[yoshisuga]

After placing breakpoints in code, yes it does look like the one inside #if LIBAVCODEC_VERSION_INT >= AV_VERSION_INT(55, 52, 0) is getting called, not the block where the code was fixed. So I think you did the real fix by updating ffmpeg.

Feel free to close this pull request as you see fit - and thanks again for all the feedback!

[unknownbrackets]

Well, I think this is likely to be more correct, so I'm going to merge.

Thanks for testing, great to hear. I don't actually have an iPhone or any other device - we could really use more help in that area if you're keen.

-[Unknown]

[yoshisuga]

No problem! I'd be glad to contribute anyway I can! Thanks again!!

[unknownbrackets]

Well, if you want to give it a shot, #6962 might be a good one if you have a newer device. Initially something was proposed in #6965, but apparently it broke the build on the buildbot and had to be reverted.

There's also #5333, if it looks interesting. Someone commented to me that it "shouldn't write files into the bundle." I don't know what that means, though.

There's also rotation (#7777), Android gives users the option to lock the orientation of the screen (e.g. Portrait instead of Landscape), but we don't have any way yet on iOS. And there's a problem (or used to be?) with controls sticking (#5099).

Not sure if any of those are interesting. There are a number of users who will definitely be very happy with you if the iOS build gets more stable.

-[Unknown]