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.
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
AviSynth demuxer rewrite patch #90
Comments
|
Can we discuss this by email? I too would be interested in completing this, but I stopped tracking this after not seeing much response from ffmpeg-devel. Please feel free to follow-up with me at avxsynth.testing@gmail.com |
|
To remark on the points you've made though:
|
|
I'm really not sure how much help I can be on this. The process of getting AvxSynth support into x264 (still in x264-devel) was mostly me being directed rather than doing anything novel myself.
I wasn't really sure how or if it could be transferred over, though. |
|
|
RGB flipping done: qyot27/FFmpeg@78f2e6e Also has another attempt at stopping the segfault. This one doesn't seem to cause the kind of resource slogging that I saw with simply commenting out the avs_delete_script_environment in read_close, and is actually instantaneous at close. This may or may not also be a consequence of using a highly stripped-down build of FFmpeg*, but I'm leaning toward the side that the minimal build isn't the real reason for the improvement. YV12 is still swapped, verified by using Version().ConvertToYV12() - the text appears with a blue tint on both Linux and Windows and using SwapUV() returns it to its proper appearance. The 2.6 colorspaces still silently fail on Windows...any suggestions on what can be done to get useful debug output on that? GDB seems to not want anything to do with dynamically-loaded avisynth on Windows. *the used configure settings: |
|
To step into avisynth.dll on Windows, you may need to use Visual Studio. It is my understanding that the latest git versions of ffmpeg/libav can be compiled in MSVC 2010 or later. Your "fix" for the segfault is a NOP. Calling free/delete on NULL does nothing in C++. Incidentally, I noticed that there was a bug in the seek routine, which I have fixed in this diff (with other changes): http://pastebin.com/vucbGbjE |
|
Applied (although I now see I forgot to remove pthreads.h too), and now AviSynth 2.6 colorspaces are supported: And actually, now that I've done more testing, YV24 actually does not have an issue. The issue seems to actually be in the yuv444p rawvideo decoder, because using yuv4mpegpipe or ffv1, YV24-bearing scripts work fine too. And finally, it seems like the segfault is now only occurring on Linux. Windows is unaffected by it. |
|
It is interesting that the 2.6 avisynth_c header is not a complete port of the 2.5 header. These lines are actually unnecessary vestiges that I forgot to take out: IIRC, the segfault occurs because of an interaction between dlclose, atexit, and C++ objects. I will look into seeing if it can be mitigated on the AvxSynth side. Edit: |
|
I'm not entirely sure of the logistics behind the 2.6 header. It's simply the one from x264, as I figure it's advisable to keep FFmpeg and x264 in sync with each other on this aspect (namely because x264's support is probably more stable over the long term than the actual development of 2.6 itself). If/when 2.6 hits final, it'd of course be the right thing to move both of them up to the canonical 2.6 header. You'll notice that there are some differences (I can only remember some organizational stuff) when comparing avisynth_c.h from x264 to the one from AviSynth CVS. Considering all the points on global objects, would that have anything to do with the inclusion of RTLD_GLOBAL in the dlopen call? It might explain why I never encountered a problem when adapting the x264 patch, since I only used RTLD_NOW. It certainly doesn't seem to have had any adverse effect on it there, but I'm not too sure what the consequences are for not using RTLD_GLOBAL in that case. Edit: Yeah, RTLD_GLOBAL has nothing to do with it. |
|
I have sat down and thought about this for a while. Unfortunately, I can't think of a way to resolve ffmpeg's atexit usage with AvxSynth's usage of global objects. Something has to give, and unfortunately, we are not willing to give up logging to avoid this segfault. This patch to ffmpeg.c replaces the offending atexit handler and does not appear to break anything else: There is a second atexit handler in libavcodec/bfin/dsputil_bfin.h which is not affected by this change as it does not touch any dynamically allocated structures (as far as I can tell). |
|
The solution I went with is to spawn a new branch with the demuxer-related commits squashed into a single patch, and the atexit fix in a second patch. https://github.com/qyot27/FFmpeg/commits/avisynth-demuxer2 I also tested the new branch on OSX (10.7 specifically), and it gives correct output (although building AvxSynth on OSX is kind of a pain due to autotools being, not right, under it; I have to resort to autoreconfing the build system in an Ubuntu VM before going native for the rest of the compilation process...). The only tweak I can think of now is that it seems as though avs_planes_yvu and its corresponding planar case are now unnecessary, since all the planar formats point to case 1, or for Y8, case 3. Otherwise, I'd say resubmit to ffmpeg-devel or I could issue a Pull Request on it. |
|
avs_planes_yvu can be removed. Please feel free to submit your branch to ffmpeg-devel on my behalf. As Avxsynth is no longer my primary responsibility, I have limited time to work on it and argue with ffmpeg maintainers. |
|
Doh! There's a horrible memory leak in the demuxer. Apparently I should set pkt->destruct to "av_destruct_packet" in avisynth_read_packet. This also improves the performance of decoding BlankClip from 80 fps to 800 fps ;) Also, I just realized that there are a bunch of exit calls in ffmpeg.c that probably should be changed to do_exit. |
|
Applied, practically all of the exit calls have been moved over to do_exit, and new branch spawned at avisynth-demuxer3 |
|
Submitted to ffmpeg-devel: |
|
Response to the atexit handling situtation: On Tue, Feb 26, 2013 at 9:56 AM, Michael Niedermayer michaelni@gmx.at wrote:
|
|
What michaelni suggests is feasible. It would involve creating a global linked list of AvisynthContexts and a global atexit_called flag. Calling avisynth_read_header will add an element to the list of contexts and a avisynth_read_close will remove it. The first call to avisynth_read_header will register with atexit a function to destroy the list of contexts. Edit: |
|
I had been writing an AVInputFormat a few months ago, but stopped when I found out from qyot that you had already written and submitted one on the mailing list. A little late, but I guess now would be the last good time to post it if you two want to have a look: Color formats should be handled correctly* and it handles interlacing as well, also avoids that mplayer bug by using avio to read the script (which has the pointless side-effect of making things like *Not sure about the Linux/Windows i420 issue. |
|
Linking to Avisynth solves a lot of problems, but it means that users would need to have Avisynth in order to run ffmpeg binaries, which would be highly unfortunate. |
|
As an aside, support for AvxSynth in x264 is now official. But regarding the issue of atexit in avisynth.c, does that mean neither AviSynth nor AvxSynth could be unloaded, or specifically just AviSynth? If it's just one and not the other, could it be special-cased/macro'ed around? |
|
In looking around, it seems that the only other library FFmpeg uses dlopen for is libavfilter's frei0r support (libavfilter/vf_frei0r.c). Might there be something in there to use as a model for this? I don't know if frei0r is subject to atexit, though. |
|
The frei0r ifmt doesn't do anything special, actually it looks like this sort of problem could manifest with a particularly written C++ frei0r filter. Dynamic loading would be convenient and would make it easier to get AvxSynth support in e.g. VLC binaries, but it seems to be causing quite a few headaches. Since linking is how every other ffmpeg dependency is handled (except frei0r which would be more akin to an AviSynth plugin loader), and it's unlike the Windows AviSynth input where there are no package managers to help, it may be worth considering. Just a thought. |
|
I guess at this point: with the situation of it not being unloaded until the program ends if avisynth.c is made to also use atexit, what is the practical consequence there? Is there really a case where the input file is unloaded and the program does not then end immediately or soon thereafter? It doesn't seem like something that would affect a program like FFmpeg very much, although I could see how it might impact media players that load a playlist if they don't free up and refresh everything between entries. The other 'solution', i.e. disabling avs_delete_script_environment in read_close (and only read_close), what are the consequences of that? Is it pretty much the same situation as above, where it just means it doesn't get unloaded until the program exits? That can easily be special-cased with an ifdef _WIN32 since regular AviSynth has no problems with it. I ask this specifically because I know that on Windows it was probably the cause of the resources getting bogged down at the end and persisting even after ffmpeg closed. On Ubuntu 12.10, though, there weren't any issues that I could immediately feel from it - and this was on the same exact hardware: a Coppermine (PIII era) Celeron with 512 MBs of PC133. And since the issue with the segfault only exists on Linux (and possibly OSX), it would seem to be pretty cut-and-dry: allow the use of avs_delete_script_environment on Windows, disable it everywhere else unless/until a better solution is found to make atexit and dlopen play together nicely. I see this as the least acceptable option on the table, but it does 'work', so I'd only see it as a last resort unless there really are only small issues with it. As it stands right now, the dynamic loading patch has no issues on Windows that I'm aware of, and the only remaining issue on Linux and OSX is this segfault conflict (and that only affects the ffmpeg binary itself or other programs that use atexit like it does - I'm about 99% sure the MPV test build I made was with a lavf that had a segfaulting FFmpeg, and MPV was totally cool with playing scripts without segfaulting). Although I guess I should ask this, does the dynamic patch have support for interlacing? |
|
I don't think separating Avisynth and Avxsynth demuxers is a good idea. Consequences of not unloading Avisynth DLL until program exit: Consequences of not deleting script environment: Path forward: Interlacing: |
It distinguishes field-based clips from frame-based, whereas ffmpeg only handles complete frames and identifies fields by a stream flag. Adding weave() was an idea from the x264 input module, otherwise the behavior is similar to using assumeframebased() all the time. |
|
Being field-based is still completely different from being interlaced. If the user returns a field-based clip from his script, that must be what he wants, and I don't see the value in second-guessing the user. |
|
Another point brought up: On Wed, Feb 27, 2013 at 9:03 AM, Michael Niedermayer michaelni@gmx.at wrote:
I'd really only feel comfortable with testing patches, not so much the rest of the maintainer duties. Also, one thing I did notice with the previous AviSynth demuxer is that it performs code page conversion (http://git.videolan.org/?p=ffmpeg.git;a=commit;h=5c742005fb7854dcfaa9f0efb65fd36a63ceaa2b). Is something like this going to be necessary, even if only for Windows? |
|
Yes, that looks like a valid concern. I would have to test to see if unicode is also an issue on Linux. It may not be, as Linux uses UTF-8 strings. If the code is accepted, I will accept responsibility for maintenance. Edit: UTF-8 filenames do not appear to be an issue on Linux. Input #0, avisynth, from '../日本語.avs': Edit2: The reason malloc/free/printf are undef'd in avisynth.c is because avisynth_c.h for some unexplained reason contains an inline function that calls malloc/free/printf, resulting in a compiler error with the ffmpeg guard macros. |
|
I have written a patch that implements the atexit handler I discussed above: This is a fairly intrusive patch, so it should probably be tested. It also fixes a minor memory leak and a potential overflow in computing the size of video packets. |
|
Not the filenames of the scripts, the filenames of the videos loaded into it. What FFMS2 is handling (or on Windows, AVISource, DirectShowSource, DSS2, the various DG* source filters, etc.). I'm pretty sure Windows can handle it if the scripts themselves have the Unicode names, but not if you're trying to load videos with Unicode names into the scripts. I figured that Linux and OSX wouldn't have an issue due to UTF-8, though. Regarding the undefs: |
|
Never mind, Windows does indeed have a problem with Unicode filenames. Even when the Command Prompt is switched over to UTF-8 with chcp 65001:
|
|
That's something that could be sidestepped by using avio_read+eval. |
|
The new atexit-handling patch works for me under Windows and Linux. OSX can't decide whether it hates me or not, but the thing I was experiencing there has nothing to do with the patch, just with dependencies deciding to explode in my face and prevent AvxSynth from doing anything. But given a setup that doesn't have that problem, I have no reason to believe it wouldn't work there if it works fine on Linux. I've posted it to ffmpeg-devel. |
|
Reading the script file and calling invoking Eval() is not a valid solution because the Avisynth current working directory would be different in that case. Based on what you found, it's probably necessary to explicitly convert the filename using some Win32 APIs. |
|
Ah true. It is possible with char *cwd = av_dirname(av_strdup(filename));
val = avs_invoke(avs->env, "SetWorkingDir", avs_new_value_string(cwd), NULL);
av_free(cwd);before the eval invocation. But I'm past quota for dubious suggestions in this thread, sorry for the noise. |
|
I think I've stumbled across a very, very strange bug. I'm not sure if it affects AvxSynth, but I'm consistently getting hangs with AviSynth under Windows under the following conditions: *Encoding audio If audio is disabled, affected scripts can output video just fine. It's like it's something particular about audio in these cases. There's no error warnings or anything (even with -v 9 -loglevel 99), ffmpeg.exe just hangs. Forcing a different framerate that's known to work using ChangeFPS suddenly makes it possible to output audio again. 12000/1000 is fine 15000/1000 is fine 23000/1000 hangs #to illustrate an arbitrary fps 24000/1000 is fine 25000/1000 is fine 30000/1000 is fine 48000/1000 is fine 50000/1000 is fine 60000/1000 is fine Checking the actual script in Windows Media Player the script is completely fine. I can pipe the audio from wavi to ffmpeg and it works. I initially found this because I was attempting to convert a 29.97fps script's audio and it just hung there for 3 minutes until I killed the Command Prompt, and the output file was empty. The rest of the scripts were 23.976 and the conversion process started instantly and finished successfully as expected. If it's a memory leak on those hanging fpses or something, the reason I notice it is probably because I have such a low amount of RAM in this computer (just 512MB). Also, should the script's audio always be getting detected as pcm_f32le? As in that wavi example, the piped audio is detected as pcm_s16le like normal. |
|
A bit of a follow-up, it seems this is ultimately triggered by using SSRC to resample the audio rate when the video rate is one of these affected fps ratios. All the scripts had been using SSRC to go from 44.1->48, but the only one that had a problem was the one at 29.97fps. Like I said before, using ChangeFPS or ffms2's fpsnum/fpsden parameters to force an fps that didn't have the issue made it work (and obviously, so did removing/commenting out SSRC). |
|
I've isolated it to the avs_has_video block in avisynth_read_packet_audio, lines 497-504. I still have no clue why it works with some framerates but not others, though (my best guess is that AviSynth's internal representation of those fps values differs from what the demuxer is expecting, e.g., you can give it exactly as 30000/1001 in the script, but it exposes it to FFmpeg as 2997/100, and then when SSRC's treatment of the audio is added into the mix, something happens that the math doesn't like). The first samples = av_rescale_q assignment @ line 499 seems to be what's responsible for the problem. The 'always showing up as pcm_f32le' was a mistake on my part. That only happens when SSRC is called because it converts the audio to float. And it is - obviously - a case of only happening with AviSynth because AvxSynth doesn't have a ported version of SSRC. Strangely, the hang only wants to occur when I test on Windows. If I test the binary under Wine, it works, although it does still exhibit the sign that it has a problem: below the 1 second mark, it goes ridiculously slowly, and then after it passes that point, it speeds back up to normal (the audio output is okay). With fpses that aren't affected by this, the initial slowdown never happens. It also affects anything using the script with libavformat, as the mpv test build showed the same problem on Windows: 23.976 and SSRC, no problem. 29.97 and SSRC, hang before decoding and playback starts. |
|
This sounds like a math error. Could you post a script that demonstrates this problem? I was unable to reproduce it with this: BlankClip(pixel_type="YV12", channels=2, audio_rate=44100, fps=29.970) I tried this with ffmpeg/mingw32 on Windows 8 with Avisynth 2.6a4. The command I ran was: ffmpeg.exe -i test.avs -r 30000/1001 test.avi |
|
Well, with BlankClip it works (and it works with AVISource and DirectShowSource). Okay, it seems it's a problem with a very particular build of FFMS2 (my branch of the C plugin + the extra planar-audio-related commits in Plorkyeran's dev branch so that FFMS2 can output audio when built against post-November 26th builds of FFmpeg), but it's still sparked by using SSRC and causes the demuxer to hang. However, I did notice that the slowness-on-start issue when using those framerates does exist with this build of r739. With the affected fpses, the progress of encoding stutters below one second for a little while, and then speeds up to normal. Non-affected fpses don't do this. Example script: no slowdown at start:
slowdown:
(also shows it when converting to pcm_s16le)
Like I said, though, the script works fine in Windows Media Player 6.4 and wavi without that kind of slowdown or hanging, and the VFW-based demuxer didn't have issues with it either. |
|
That sounds like your problem and not mine. Edit: I managed to load the first plugin you linked (http://www.mediafire.com/?w2t2cvddnxcmhc0). I still don't observe the issue. Edit2: I managed to reproduce the issue. The problem seems to be that ffms crashes when attempting to read certain numbers of samples. This is a bug in ffms that should be fixed. 525 pkt->size = avs_bytes_per_channel_sample(avs->vi) * samples * avs->vi->nchannels; Program received signal SIGSEGV, Segmentation fault. As for why the avisynth demuxer is attempting to read odd numbers of samples: |
|
Ok, that puts my mind at ease, although I'd have thought that whatever SSRC is doing would be external to the way FFMS2 is handling it. I'll see about generating info with an actual debug build and submit it on FFMS2's side. Regarding the Unicode filenames issue, I did happen upon a function (ff_win32_open) in libavformat/os_support.c that seems like it deals with this sort of thing (and shares a bit of similarity with the patch that was submitted for the old AviSynth demuxer). I'd tried to see how to adapt it, but I kept getting Access Violation errors whenever I tried testing it under Wine. |
|
Reported on FFMS2's side. It's a little strange that I didn't get the same error message from the backtrace, though. Probably has to do with how I ran gdb. |
|
I've thought about unicode filenames for a while, and stumbled across this thread: http://forum.doom9.org/showthread.php?t=110467 The summary is that Avisynth accepts filenames encoded in the local codepage. From the point of view of the ffmpeg demuxer, this means that the string passed in avisynth_open_file to avs_invoke("Import", filename) must be encoded in the local codepage. The question is what FFmpeg does internally with strings from the console -- whether it holds them as-is, converts them to UTF-8, or converts them to UTF-16. If it is either of the latter two cases, the string should be converted to the local codepage by a Win32 call. Note that it is still impossible to open files from other codepages than the local one, e.g. opening a BIG5 script file from a ISO-WESTERN system. |
|
Looking at what ff_win32_open does, the MultiByteToWideChar call is what handles it. It's assuming UTF-8 and then as per the documentation from Microsoft on that function, converts it to UTF-16. In the old demuxer, this was handled by following the MultiByteToWideChar call with WideCharToMultiByte, converting UTF-16 to the local code page and handing that over to the rest of the VfW stuff. So it would appear the path was UTF-8 filename -> UTF-16 -> Local code page -> proceed. So now I'm wondering if those Access Violations I got under Wine weren't because it was working incorrectly, but because it was trying to convert back to the local code page on the larger Linux environment, i.e. UTF-8, and because the function and/or AviSynth can't handle that, it errored out (and at the same time, using it under real Windows might have worked because the local code page there isn't Unicode). EDIT: Nope, the Access Violation is not indicative of just an issue with Wine. ffmpeg.exe crashes under real Windows, with no error message from AviSynth like on Wine. I'll push the [non-working] bit that I have done up so that it can be critiqued, though. Maybe it's not far off from being workable. |
|
So, um, it got committed. Took me by surprise since I'd been trying to get a couple of other things hashed out (aside from the UTF-8 issue, I found out that MSVC does not like it one bit, even though it compiled, the resultant build crashed when AviSynth scripts were handed to it), but I suppose that those things will get noticed by those that need them soon enough and patches to fix them will appear. The thing that remains is how you want to be credited in MAINTAINERS. |
|
Nevermind about the MSVC thing, I rebuilt it just now and it works. Either it was fixed in git already (the previous build was a few days-a week old), or it was just some configuration issue that got resolved by my mucking about in VS2010 for a different project to try and get it to build. |
|
The maintainers file can credit AvxSynth team for managing avisynth.c. Edit: In the ffmpeg push, I noticed this line in avisynth_context_create 182 av_free(avs); This line should have been removed, and I swear it was in one of the many versions of the patch posted here. Edit2: Actually, I don't know what I was smoking. The function should read: This constructor function was not correctly updated after I made the changes for the atexit handler. I guess I/we should have used source control when writing this. |
|
Seeing as how this has been accepted into upstream, closing this as resolved. |
|
There is a particularly nasty issue with AviSynth 2.5.8: |
Posting this here because it's sort of related.
I wanted to see about making the October 9th patch from FFmpeg-devel work against git master. The result is here:
qyot27/FFmpeg@9087175
On Linux and Windows, AvxSynth and AviSynth have issues with YV12's channel order, always assuming I420. On OS X, it's correct for some reason.
RGB24 and RGB32 are upside-down on all platforms.
The segfault on exit seems to have been caused by the combination of avs_delete_script_environment and avisynth_free_library being used in read_close. Disabling avs_delete_script_environment stops the segfault, but more than likely causes leakage.
AviSynth 2.6's colorspaces don't work when using AviSynth 2.6, failing with exception handling errors.
The text was updated successfully, but these errors were encountered: