Fix thumbnail creation for ffmpeg versions without libavfilter and no… #76
Conversation
…n-keyframe skip options
|
This issue was discovered in Linux V9. In this fix, instead of checking the OS type, I am calling ffmpeg to check the supported options and making a decision based on that. My concern is that we don't know for sure which version of ffmpeg someone has. I checked my old (archived) 7.1.9 Linux 32-bit installation and that ffmpeg actually did support -vf but the one included in V9 does not. So depending on how someone upgrades to V9 on Linux they could potentially have either one. I also noticed that the older ffmpeg version does not support the skip non-keyframe options that SageTV is trying to use during the first attempt to create the thumbnail, so I'm checking that too in order to handle it more gracefully than just executing ffmpeg and having it fail. SageTV tries twice to create the thumbnail, using different options each time (and without the skip options the second time). I tested the fix on Linux 64-bit (clean V9 install) and Windows (upgraded from 7.1.9 using JAR and STV) and it's working fine. |
|
One thought that I had here is that we are going to execute ffmpeg twice for every thumbnail (one to check the arg list, and and the second time to create the thumbnail). This is probably not too bad if we are only doing a single thumbnail per recorded video, but there there a places in Phoenix (and probably gemstone) where we ask sage to create 100s of thumbnails (creating a filmstrip). Should we cache the arg check result so that we don't have to do it again. Maybe a static field on the MediaFile object? or a Static field on the Sage object? |
|
Good point...it would be better to optimize it. Would it be OK to just define the static fields as null and keep the arg check code inside execVideoThumbGen and set the values only if the fields are null, or would it be better to execute the arg check initially when the static fields are defined in MediaFile (but outside of execVideoThumbGen)? I've seen both approaches used throughout the code. |
|
Personally, I'm ok if you define them as null and then just set them the first time you go to create a thumbnail. Jeff might want you to synchronize this... but I'm more carefree and would be willing to run the risk that 2 threads would try to update the static field at the same time. |
|
Sounds good, and using synchronized is probably a good idea just to be safe. I'll make the additional changes and test them in Linux and Windows. What is the preferred method for submitting additional changes to a pull request? Is it OK to just create an additional commit? |
|
yeah, you can just continue to add commits, and they'll automatically show up in the pull request. |
|
I've made the changes suggested by stuckless. Looking closer at the MediaFile code, it appears to be using a queue to execute the thumbnail generations one at a time (imageLoader Runnable objects) but I don't know if it's possible for SageTV to create multiple ThumbnailGen threads. Maybe Jeff or someone more familiar with the code can provide some feedback? |
| @@ -5486,6 +5489,29 @@ public String execVideoThumbGen(String pathString, boolean deinterlace, float of | |||
| int targetStream) | |||
| { | |||
| List<String> args = new ArrayList<String>(25); | |||
| String res; | |||
| synchronized (this) | |||
There was a problem hiding this comment.
Synchronizing around 'this' doesn't really have much purpose here as it's dealing with static variables. You can just drop the synchronization altogether since it's completely safe to run the test multiple times in parallel because they'll end up setting them to the same value.
|
I'm fairly sure there's some kind of mechanism which is restricting it from doing multiple thumbnail generations at once...but that got moved around through the system over the years so I don't recall exactly how that's handled at the moment. :) I had a few comments on this code; so just clean that stuff up and then I can approve the change. |
|
Thanks for the feedback. Regarding the synchronization, I figured it would avoid unnecessary setting of the values. But you are right, we can probably get away without it since setting the values again won't cause any problem other than some extra executions of ffmpeg (once per thread, worst case). Should the static field also be volatile to make sure other threads immediately see the change to the variable? The reason I am bailing out when the skip options are not supported is because the calling code is also setting the offsetTime to non-zero when SkipNonKeys is enabled and that leads to the minpix options being set differently when executing ffmpeg. What I found in testing is that if the skip options are not supported, the thumbnail must be taken with an offset of 0 and with minpixenergy of 1 or else the thumbnail seems to always catch a non-keyframe and looks pixelated and distorted. So by bailing out, the existing logic in the calling code executes execVideoThumbGen again with SkipNonKeys as false and an offsetTime of 0 and that works correctly with the older ffmpeg. I was trying to avoid moving some of the offset logic into execVideoThumbGen and having it in two places. But if you prefer, I can avoid the second call to execVideoThumbGen in these cases by forcing the offsetTime to 0 when ffmpeg does not support the skip options and then also add an ffmpegSupportsSkipNonKeys condition to the code block that sets the minpix options so it uses the minpixenergy option. Then the first call to execVideoThumbGen should always succeed for the older ffmpeg (unless there is an issue in the stream) but I'm not sure if this approach overly complicates the code or makes it more confusing to follow. |
|
You can go ahead and mark them volatile...nothing wrong with that. And good catch on the corrupted frames...I forgot about that case which is likely the other reason I put those options in there. :) Regarding bailing on skipNonKeys; you're right that in the MediaFile class it will retry...but it doesn't retry in the MediaFileAPI class when it invokes execVideoThumbGen. I agree with not moving the retry logic into execVideoThumbGen itself; because there's different actions it may want to take when it retries. Such as in the MediaFileAPI; where if it fails, then I wouldn't want to reset the offset time to be zero since that would likely totally violate the intention of the caller in that scenario. ...Jeff goes and does some research... Now I think there's something else I should fix instead. I was under the assumption that the FFMPEG version I had put into GitHub was newer than the one that was released with V7. But that's apparently wrong...this is what's in GitHub: libavutil version: 49.6.0 And this is what's in a V7 install of SageTV: libavutil 50.22. 0 / 50.22. 0 So now I need to figure out why the FFMPEG version in GitHub is not the newest one...and where the hell that newer version is at. :) The fact that my Windows and Linux builds both have the same versions means it really should be in source control. I'll dig into this and report back...upgrading to the proper FFMPEG version would be a way better fix for this problem. |
|
Ohhh....I think I found it: http://download.sagetv.com/pubcode/ And I recall what happened now. At one point our FFMPEG versions that we used on our embedded devices diverged somewhat from desktop; and we kept the embedded device FFMPEG in our CVS repo with all the other code...and that's what the GitHub one is based off. I totally forgot about the SVN repo we had which housed the FFMPEG and MPlayer code that we used (although I think the GitHub MPlayer code matches what's in SVN since we didn't mess with that as much in the later years). I'm quite sure I still have the SVN repo on a NAS here; and I'll find that and make sure the MPlayer code matches as well. I'll also then update the FFMPEG codebase in GitHub so that it matches what as in V7....and then this patch can likely be abandoned. Sorry for all the confusion on this...I got confused myself on what had happened...but now it's all clear and I will correct this. :) |
|
No problem, and thanks for the research! Let me know if you prefer that I wait while you work on this or if I should proceed with the additional changes. I could modify the code to only change the offset to 0 when the skip options are not supported and keep the retry logic outside of execVideoThumbGen--as long as that won't break the usage from MediaFileAPI. But obviously getting the correct FFMPEG version is best. We'll just need to make sure that people who already have the wrong version fix their installations. |
|
Don't spend anymore time on this one...once I have the proper FFMPEG version in there, everything should be working fine (and I'm pretty sure I'll have to put some of the codecs back on the right version as well because apparently some of those are different now that I've got the right FFMPEG in there...fun, fun, fun :) ) |
|
Sounds good. I'll change gears and focus on submitting my other pending fix (for FreetypeFont). :) Should we close/cancel this pull request so my next commit can be submitted as a new pull request? |
|
Feel free to cancel this one...although you should be able to just do another branch separately and submit a pull request there. There doesn't need to be dependencies between the two. |
|
This is going to take me a little longer than I thought... The version we have on download.sagetv.com/pubcode matches the FFMPEG format numbers in our V7 distributions. However, the latest version in the SVN repo we had...is older than that....which doesn't make any sense at all. I think I need to revive my old workstation...that will have all the answers on it. :) |
|
FFMPEG has been updated now so I'm going to close this one out as it should have the proper options in it and match what was in V7. |
…n-keyframe skip options