Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
Quoting David Bryant (2020-04-06 06:11:17) > On 4/2/20 11:32 PM, Anton Khirnov wrote: > > > >>> Also, I looked at the specification PDF you linked in the previous email > >>> and I see nothing in it that explicitly forbids the channel count from > >>> changing from one "superblock" to the next. It seems to me like you > >>> could just concatenate wavpack files with different channel counts and > >>> get a valid wavpack file. > >>> > >> You're right, I could not find that being explicitly forbidden. > >> Nevertheless, it _is_ forbidden for any stream parameters to > >> change (e.g. data format, sampling rate, channel count or mapping) and > >> both libwavpack and the FFmpeg demuxer enforce this (see > >> libavformat/wvdec.c starting at line 211), or at least gracefully fail > >> without crashing. > > Forbidden by whom? From what I can see: > > - sample format: the wv demuxer does not set it at all, the decoder sets > > it in wavpack_decode_frame() from the block header; also, Michael sent > > a patch recently to deal with a wavpack decoder crash related to > > format changes > > - sample rate: set by wv demuxer and then checked that it does not > > change; but the caller is not necessarily using the wv demuxer and the > > decoder will happily change it the same way it does with channel count > > > >> The reason it's never mentioned is probably just because it never occurred > >> to me. None of the source file formats for WavPack > >> (e.g., wav, caf, dsdiff) allow format changes and neither do any of the > >> other lossless compressors I am aware of. > > Correct, I cannot see any other lossless codecs that would allow this. > > But then again all these codecs have global headers. So it's not that > > they artificially forbid format changes, but the bistream format is not > > able to represent them. > > I disagree with that. It would be trivial to add a new STREAMINFO block to a > FLAC file and change the format after that. This is > not specifically prohibited in the FLAC specs, but it's sort of implied when > it says that the initial STREAMINFO block has > "information about the whole stream". From a quick glance at the spec, metadata blocks are only allowed at the beginning and are followed by audio frames. Once you see a single audio frame, there should be no further metadata blocks. That said, the frame header does contain stream parameters and those can conceivably change and after looking at our decoder, it does support those changes. It was added explicitly in commit 90fcac0e95b7d266c148a86506f301a2072d9de3 Author: Justin Ruggles Date: Sun Oct 21 17:02:28 2012 -0400 flacdec: allow mid-stream channel layout change Although the libFLAC decoder cannot handle such a change, it is allowed by the spec and could potentially occur with live streams. > > > > >> and extensive fuzzing has ensured that pathological files like you > >> suggest are unlikely to be the source of exploits. > > See the crash above. I think fuzzing is very overhyped these days. It's > > a useful tool for sure, but one should not rely purely on fuzzing too > > much. It may not cover important parts of the input space because of the > > way it is set up. Or there could be exploitable codepaths that require > > too specific inputs for a random process to find, but a determined > > attacker could construct them from reading the code. > > > Of course fuzzing cannot find everything, but my experience has been that > fuzzing finds things that I would _never_ find by just > looking at the code. > > And I was certainly not suggesting that one should just write sloppy code > knowing that all the bugs will be found with fuzzing. Yeah, I certainly agree that fuzzing is a very useful tool. My point is just that it shouldn't be your _only_ tool. There can be plenty of bugs fuzzing won't find. -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 4/2/20 11:32 PM, Anton Khirnov wrote: > Quoting David Bryant (2020-04-03 07:14:21) >> On 4/2/20 2:13 AM, Anton Khirnov wrote: >>> Quoting David Bryant (2020-04-01 23:35:13) On 3/31/20 2:47 AM, Anton Khirnov wrote: >>> Looking at wavpack, the code looks suspicious to me. You allocate one >>> dsdctx per channel at init, but AFAIU the number of channels can change >>> at each frame. >>> >> Multichannel WavPack consists of sequences of stereo or mono WavPack >> blocks that include flags indicating whether they are the >> beginning or end of a "frame" or "packet". This allows the number of >> channels available to be essentially unlimited, however the >> total number of channels in a stream may not change. When decoding, if a >> sequence of blocks generates more or less than the >> correct number of channels, this is flagged as an error and overrun is >> prevented (see libavcodec/wavpack.c, line 1499 and line >> 1621). > If my reading of the code is correct, line 1499 only checks for overflow > of the channel count read for the current sequence. That channel count > is exported to AVCodeContext in the block starting on line 1464 and > I don't see any checks for whether it is equal to the initial one. > Full disclosure: I did not write the WavPack decoder. I understand _how_ it works, but I don't necessarily understand _why_ it was written the way it was due to gaps in my knowledge of the FFmpeg internals, and there are certainly things in there that don't look quite right to me. That said, I have tested it thoroughly and it handles everything I throw at it, and I have extended it to handle the DSD mode without anything unexpected happening. And of course it seems to handle extensive fuzzing, and the recent error was related to my DSD change, suggesting that the core it pretty robust. My inclination has been to not "fix" anything that wasn't broken. Yes, that channel count stuff is a little difficult to follow. I could experiment with streams that abruptly change channel configuration, but again I'm hesitant to change code that's worked so well so long. >>> I can easily get the decoder to crash even in single-threaded mode. Just >>> make the demuxer always say the stream is mono, then try decoding your >>> DSD sample. >> Okay, I reproduced that. However, I'm not sure it's really that serious >> because the demuxer and the decoder use basically the >> same logic to determine that number of channels, so it would be tricky to >> craft a file that caused the demuxer to calculate 1 >> channel and the decoder to reach a different result, which is probably why >> the fuzzers have not been able to do so. > I disagree - this is just as serious as any other crash on invalid data. > > The decoder must not ever crash for arbitrary input data. It cannot > assume that it's always going to be provided valid input or that it's > always used with some specific demuxer. The caller might not be using > libavformat at all, libavcodec should work with any source of data. > > Besides, I can reproduce the crash with no code modification necessary > by muxing your sample into Matroska and then hex-editing the file to say > it has only one channel. Yes, I have reproduced this crash even without Matroska. I was working on a solution but you beat me to it. Thanks! > >> That said, it would not be hard to have the decoder save away the initial >> channel count from avctx, never overwrite it, and >> verify that all superblocks match that count. I don't know off hand whether >> that would break anything though. Is it guaranteed >> that avctx->channels is correct during the call to decode_init(), even if >> it's being demuxed from Matroska, for example? If so, >> why is the WavPack decoder _ever_ writing to the avctx channel count? > The question is what you define as correct. The initial value provided > by the caller is supposed to be the caller's best guess at the correct > (initial) value. It may come from a demuxer, or from the Delphic Oracle. > The decoder doesn't know. > > For some codecs, the relevant parameters are not specified in the > bistream, so they have no choice but to use the caller's value. That is > not the case for WavPack, since the channel count IS written in the > bistream. So I think it's best to ignore the caller-provided value and > always use the one we get from the data. Agreed. > >>> Also, I looked at the specification PDF you linked in the previous email >>> and I see nothing in it that explicitly forbids the channel count from >>> changing from one "superblock" to the next. It seems to me like you >>> could just concatenate wavpack files with different channel counts and >>> get a valid wavpack file. >>> >> You're right, I could not find that being explicitly forbidden. >> Nevertheless, it _is_ forbidden for any stream
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
Quoting David Bryant (2020-04-03 07:14:21) > On 4/2/20 2:13 AM, Anton Khirnov wrote: > > Quoting David Bryant (2020-04-01 23:35:13) > >> On 3/31/20 2:47 AM, Anton Khirnov wrote: > > Looking at wavpack, the code looks suspicious to me. You allocate one > > dsdctx per channel at init, but AFAIU the number of channels can change > > at each frame. > > > Multichannel WavPack consists of sequences of stereo or mono WavPack > blocks that include flags indicating whether they are the > beginning or end of a "frame" or "packet". This allows the number of > channels available to be essentially unlimited, however the > total number of channels in a stream may not change. When decoding, if a > sequence of blocks generates more or less than the > correct number of channels, this is flagged as an error and overrun is > prevented (see libavcodec/wavpack.c, line 1499 and line > 1621). > >>> If my reading of the code is correct, line 1499 only checks for overflow > >>> of the channel count read for the current sequence. That channel count > >>> is exported to AVCodeContext in the block starting on line 1464 and > >>> I don't see any checks for whether it is equal to the initial one. > >>> > >> Full disclosure: I did not write the WavPack decoder. I understand _how_ > >> it works, but I don't necessarily understand _why_ it > >> was written the way it was due to gaps in my knowledge of the FFmpeg > >> internals, and there are certainly things in there that > >> don't look quite right to me. > >> > >> That said, I have tested it thoroughly and it handles everything I throw > >> at it, and I have extended it to handle the DSD mode > >> without anything unexpected happening. And of course it seems to handle > >> extensive fuzzing, and the recent error was related to > >> my DSD change, suggesting that the core it pretty robust. My inclination > >> has been to not "fix" anything that wasn't broken. > >> > >> Yes, that channel count stuff is a little difficult to follow. I could > >> experiment with streams that abruptly change channel > >> configuration, but again I'm hesitant to change code that's worked so well > >> so long. > > I can easily get the decoder to crash even in single-threaded mode. Just > > make the demuxer always say the stream is mono, then try decoding your > > DSD sample. > > Okay, I reproduced that. However, I'm not sure it's really that serious > because the demuxer and the decoder use basically the > same logic to determine that number of channels, so it would be tricky to > craft a file that caused the demuxer to calculate 1 > channel and the decoder to reach a different result, which is probably why > the fuzzers have not been able to do so. I disagree - this is just as serious as any other crash on invalid data. The decoder must not ever crash for arbitrary input data. It cannot assume that it's always going to be provided valid input or that it's always used with some specific demuxer. The caller might not be using libavformat at all, libavcodec should work with any source of data. Besides, I can reproduce the crash with no code modification necessary by muxing your sample into Matroska and then hex-editing the file to say it has only one channel. > > That said, it would not be hard to have the decoder save away the initial > channel count from avctx, never overwrite it, and > verify that all superblocks match that count. I don't know off hand whether > that would break anything though. Is it guaranteed > that avctx->channels is correct during the call to decode_init(), even if > it's being demuxed from Matroska, for example? If so, > why is the WavPack decoder _ever_ writing to the avctx channel count? The question is what you define as correct. The initial value provided by the caller is supposed to be the caller's best guess at the correct (initial) value. It may come from a demuxer, or from the Delphic Oracle. The decoder doesn't know. For some codecs, the relevant parameters are not specified in the bistream, so they have no choice but to use the caller's value. That is not the case for WavPack, since the channel count IS written in the bistream. So I think it's best to ignore the caller-provided value and always use the one we get from the data. > > > > > Also, I looked at the specification PDF you linked in the previous email > > and I see nothing in it that explicitly forbids the channel count from > > changing from one "superblock" to the next. It seems to me like you > > could just concatenate wavpack files with different channel counts and > > get a valid wavpack file. > > > You're right, I could not find that being explicitly forbidden. Nevertheless, > it _is_ forbidden for any stream parameters to > change (e.g. data format, sampling rate, channel count or mapping) and both > libwavpack and the FFmpeg demuxer enforce this (see > libavformat/wvdec.c starting at line 211), or at least gracefully fail
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 4/2/20 2:13 AM, Anton Khirnov wrote: > Quoting David Bryant (2020-04-01 23:35:13) >> On 3/31/20 2:47 AM, Anton Khirnov wrote: > Looking at wavpack, the code looks suspicious to me. You allocate one > dsdctx per channel at init, but AFAIU the number of channels can change > at each frame. > Multichannel WavPack consists of sequences of stereo or mono WavPack blocks that include flags indicating whether they are the beginning or end of a "frame" or "packet". This allows the number of channels available to be essentially unlimited, however the total number of channels in a stream may not change. When decoding, if a sequence of blocks generates more or less than the correct number of channels, this is flagged as an error and overrun is prevented (see libavcodec/wavpack.c, line 1499 and line 1621). >>> If my reading of the code is correct, line 1499 only checks for overflow >>> of the channel count read for the current sequence. That channel count >>> is exported to AVCodeContext in the block starting on line 1464 and >>> I don't see any checks for whether it is equal to the initial one. >>> >> Full disclosure: I did not write the WavPack decoder. I understand _how_ it >> works, but I don't necessarily understand _why_ it >> was written the way it was due to gaps in my knowledge of the FFmpeg >> internals, and there are certainly things in there that >> don't look quite right to me. >> >> That said, I have tested it thoroughly and it handles everything I throw at >> it, and I have extended it to handle the DSD mode >> without anything unexpected happening. And of course it seems to handle >> extensive fuzzing, and the recent error was related to >> my DSD change, suggesting that the core it pretty robust. My inclination has >> been to not "fix" anything that wasn't broken. >> >> Yes, that channel count stuff is a little difficult to follow. I could >> experiment with streams that abruptly change channel >> configuration, but again I'm hesitant to change code that's worked so well >> so long. > I can easily get the decoder to crash even in single-threaded mode. Just > make the demuxer always say the stream is mono, then try decoding your > DSD sample. Okay, I reproduced that. However, I'm not sure it's really that serious because the demuxer and the decoder use basically the same logic to determine that number of channels, so it would be tricky to craft a file that caused the demuxer to calculate 1 channel and the decoder to reach a different result, which is probably why the fuzzers have not been able to do so. That said, it would not be hard to have the decoder save away the initial channel count from avctx, never overwrite it, and verify that all superblocks match that count. I don't know off hand whether that would break anything though. Is it guaranteed that avctx->channels is correct during the call to decode_init(), even if it's being demuxed from Matroska, for example? If so, why is the WavPack decoder _ever_ writing to the avctx channel count? > > Also, I looked at the specification PDF you linked in the previous email > and I see nothing in it that explicitly forbids the channel count from > changing from one "superblock" to the next. It seems to me like you > could just concatenate wavpack files with different channel counts and > get a valid wavpack file. > You're right, I could not find that being explicitly forbidden. Nevertheless, it _is_ forbidden for any stream parameters to change (e.g. data format, sampling rate, channel count or mapping) and both libwavpack and the FFmpeg demuxer enforce this (see libavformat/wvdec.c starting at line 211), or at least gracefully fail without crashing. The reason it's never mentioned is probably just because it never occurred to me. None of the source file formats for WavPack (e.g., wav, caf, dsdiff) allow format changes and neither do any of the other lossless compressors I am aware of. I can't imagine how that would work, and extensive fuzzing has ensured that pathological files like you suggest are unlikely to be the source of exploits. Kind regards, David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
Quoting David Bryant (2020-04-01 23:35:13) > On 3/31/20 2:47 AM, Anton Khirnov wrote: > >>> Looking at wavpack, the code looks suspicious to me. You allocate one > >>> dsdctx per channel at init, but AFAIU the number of channels can change > >>> at each frame. > >>> > >> Multichannel WavPack consists of sequences of stereo or mono WavPack > >> blocks that include flags indicating whether they are the > >> beginning or end of a "frame" or "packet". This allows the number of > >> channels available to be essentially unlimited, however the > >> total number of channels in a stream may not change. When decoding, if a > >> sequence of blocks generates more or less than the > >> correct number of channels, this is flagged as an error and overrun is > >> prevented (see libavcodec/wavpack.c, line 1499 and line > >> 1621). > > If my reading of the code is correct, line 1499 only checks for overflow > > of the channel count read for the current sequence. That channel count > > is exported to AVCodeContext in the block starting on line 1464 and > > I don't see any checks for whether it is equal to the initial one. > > > Full disclosure: I did not write the WavPack decoder. I understand _how_ it > works, but I don't necessarily understand _why_ it > was written the way it was due to gaps in my knowledge of the FFmpeg > internals, and there are certainly things in there that > don't look quite right to me. > > That said, I have tested it thoroughly and it handles everything I throw at > it, and I have extended it to handle the DSD mode > without anything unexpected happening. And of course it seems to handle > extensive fuzzing, and the recent error was related to > my DSD change, suggesting that the core it pretty robust. My inclination has > been to not "fix" anything that wasn't broken. > > Yes, that channel count stuff is a little difficult to follow. I could > experiment with streams that abruptly change channel > configuration, but again I'm hesitant to change code that's worked so well so > long. I can easily get the decoder to crash even in single-threaded mode. Just make the demuxer always say the stream is mono, then try decoding your DSD sample. Also, I looked at the specification PDF you linked in the previous email and I see nothing in it that explicitly forbids the channel count from changing from one "superblock" to the next. It seems to me like you could just concatenate wavpack files with different channel counts and get a valid wavpack file. -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/31/20 2:47 AM, Anton Khirnov wrote: > Quoting David Bryant (2020-03-28 21:22:40) >> On 3/28/20 6:23 AM, Anton Khirnov wrote: >>> Quoting David Bryant (2020-03-27 23:51:19) On 3/27/20 5:57 AM, Anton Khirnov wrote: > The current design, where > - proper init is called for the first per-thread context > - first thread's private data is copied into private data for all the > other threads > - a "fixup" function is called for all the other threads to e.g. > allocate dynamically allocated data > is very fragile and hard to follow, so it is abandoned. Instead, the > same init function is used to init each per-thread context. Where > necessary, AVCodecInternal.is_copy can be used to differentiate between > the first thread and the other ones (e.g. for decoding the extradata > just once). > --- I'm not sure I fully understand this change. You mention that AVCodecInternal.is_copy can be used where different treatment might be necessary for subsequent threads, and I see that done in a couple places, but in most cases you have simply deleted the init_thread_copy() function even when it's not at all obvious (to me anyway) that that won't break the codec. >>> In most cases, just deleting init_thread_copy() is the right thing to >>> do. E.g. all decoders that do not implement update_thread_context() have >>> to be intra-only, so every frame thread is effectively a completely >>> standalone decoder. And in most of the more complex decoders (like h264) >>> the important parameters are dynamically changeable during decoding, so >>> not that much is done in decoder init beyond allocating some stuff that >>> does not depend on the bistream properties. >>> >>> My intent is for each frame-thread worker to be treated inasmuch as >>> possible as a standalone decoder, and where it has to share data with >>> other threads to make this sharing explicit (rather than implicit as is >>> the case now). >> Yes, this makes sense. The confusing part is when the decode_init() function >> looks completely different than the >> init_thread_copy() function. This is often because the decode_init() >> function is generating things (tables, etc.) from scratch >> and the init_thread_copy() is just copying the necessary things from the >> original. In cases where the original generation might >> be time consuming this might make sense, but usually it's probably just >> making the code more complex and difficult to follow >> (which I believe was your original point). > Yes, precisely. I believe for obscure codecs it's better to waste a > little memory when doing threaded decoding, in return for simpler code. > And if people really care about being as memory-efficient as possible > then doing explicit refcounting on allocated buffers is still possible. > >> One possible interim solution for complex cases that break would be to leave >> the init_thread_copy() function there, but instead >> of having it in the AVCodec struct and called from outside (which is no >> longer possible), it simply gets called first thing from >> decode_init() if AVCodecInternal.is_copy is set. That way the architecture >> is cleaned up now, and the codec won't break and can >> be cleaned up when time permits. Just a thought. > I don't really see much point in any interim solutions, since no codecs > should break. You found a problem with wavpack, that's going to be fixed > in the next iteration of this patch. > Speaking of which, ideally we should have a FATE test for DSD files so > that I would have found the failure myself when running test. I have created a WavPack DSD test file that exercises all three of the DSD modes (fast, high, and copy). I have also verified that it would have caught this change, although I had to specify fate multithreading once I discovered it wasn't the default. If someone can add this to the fate suite (in the wavpack/lossless folder) then I can supply a patch to add the test: file: http://www.wavpack.com/dsd.wv (MD5: 74b2181f3e9829d9a5b98edd037984ac) decoded MD5 (f32le): a3a88bba95f809025dce01ecb9064091 Specifically this will break WavPack because now it will be allocating a new dsdctx for every thread instead of sharing one between all threads. I am happy to fix and test this case once the patch goes in, but is the intent of this patch that the respective authors will find and fix all the breakages? Or did I just happen to catch the one case you missed? >>> I certainly intended to convert the decoders correctly myself, >>> apparently I didn't pay enough attention to the recent wavpack changes. >>> Hopefully the other conversions are correct, but I will go through the >>> changes again to check. >>> >>> Looking at wavpack, the code looks suspicious to me. You allocate one >>> dsdctx per channel at init, but AFAIU the number of channels can change >>> at each frame. >>> >> Multichannel WavPack consists of
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
Quoting David Bryant (2020-03-28 21:22:40) > On 3/28/20 6:23 AM, Anton Khirnov wrote: > > Quoting David Bryant (2020-03-27 23:51:19) > >> On 3/27/20 5:57 AM, Anton Khirnov wrote: > >>> The current design, where > >>> - proper init is called for the first per-thread context > >>> - first thread's private data is copied into private data for all the > >>> other threads > >>> - a "fixup" function is called for all the other threads to e.g. > >>> allocate dynamically allocated data > >>> is very fragile and hard to follow, so it is abandoned. Instead, the > >>> same init function is used to init each per-thread context. Where > >>> necessary, AVCodecInternal.is_copy can be used to differentiate between > >>> the first thread and the other ones (e.g. for decoding the extradata > >>> just once). > >>> --- > >> I'm not sure I fully understand this change. You mention that > >> AVCodecInternal.is_copy can be used where different treatment > >> might be necessary for subsequent threads, and I see that done in a couple > >> places, but in most cases you have simply deleted the > >> init_thread_copy() function even when it's not at all obvious (to me > >> anyway) that that won't break the codec. > > In most cases, just deleting init_thread_copy() is the right thing to > > do. E.g. all decoders that do not implement update_thread_context() have > > to be intra-only, so every frame thread is effectively a completely > > standalone decoder. And in most of the more complex decoders (like h264) > > the important parameters are dynamically changeable during decoding, so > > not that much is done in decoder init beyond allocating some stuff that > > does not depend on the bistream properties. > > > > My intent is for each frame-thread worker to be treated inasmuch as > > possible as a standalone decoder, and where it has to share data with > > other threads to make this sharing explicit (rather than implicit as is > > the case now). > > Yes, this makes sense. The confusing part is when the decode_init() function > looks completely different than the > init_thread_copy() function. This is often because the decode_init() function > is generating things (tables, etc.) from scratch > and the init_thread_copy() is just copying the necessary things from the > original. In cases where the original generation might > be time consuming this might make sense, but usually it's probably just > making the code more complex and difficult to follow > (which I believe was your original point). Yes, precisely. I believe for obscure codecs it's better to waste a little memory when doing threaded decoding, in return for simpler code. And if people really care about being as memory-efficient as possible then doing explicit refcounting on allocated buffers is still possible. > > One possible interim solution for complex cases that break would be to leave > the init_thread_copy() function there, but instead > of having it in the AVCodec struct and called from outside (which is no > longer possible), it simply gets called first thing from > decode_init() if AVCodecInternal.is_copy is set. That way the architecture is > cleaned up now, and the codec won't break and can > be cleaned up when time permits. Just a thought. I don't really see much point in any interim solutions, since no codecs should break. You found a problem with wavpack, that's going to be fixed in the next iteration of this patch. Speaking of which, ideally we should have a FATE test for DSD files so that I would have found the failure myself when running test. > > > > >> Specifically this will break WavPack because now it will be allocating a > >> new dsdctx for every thread instead of sharing one > >> between all threads. I am happy to fix and test this case once the patch > >> goes in, but is the intent of this patch that the > >> respective authors will find and fix all the breakages? Or did I just > >> happen to catch the one case you missed? > > I certainly intended to convert the decoders correctly myself, > > apparently I didn't pay enough attention to the recent wavpack changes. > > Hopefully the other conversions are correct, but I will go through the > > changes again to check. > > > > Looking at wavpack, the code looks suspicious to me. You allocate one > > dsdctx per channel at init, but AFAIU the number of channels can change > > at each frame. > > > Multichannel WavPack consists of sequences of stereo or mono WavPack blocks > that include flags indicating whether they are the > beginning or end of a "frame" or "packet". This allows the number of channels > available to be essentially unlimited, however the > total number of channels in a stream may not change. When decoding, if a > sequence of blocks generates more or less than the > correct number of channels, this is flagged as an error and overrun is > prevented (see libavcodec/wavpack.c, line 1499 and line > 1621). If my reading of the code is correct, line 1499 only checks for
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/28/20 6:23 AM, Anton Khirnov wrote: > Quoting David Bryant (2020-03-27 23:51:19) >> On 3/27/20 5:57 AM, Anton Khirnov wrote: >>> The current design, where >>> - proper init is called for the first per-thread context >>> - first thread's private data is copied into private data for all the >>> other threads >>> - a "fixup" function is called for all the other threads to e.g. >>> allocate dynamically allocated data >>> is very fragile and hard to follow, so it is abandoned. Instead, the >>> same init function is used to init each per-thread context. Where >>> necessary, AVCodecInternal.is_copy can be used to differentiate between >>> the first thread and the other ones (e.g. for decoding the extradata >>> just once). >>> --- >> I'm not sure I fully understand this change. You mention that >> AVCodecInternal.is_copy can be used where different treatment >> might be necessary for subsequent threads, and I see that done in a couple >> places, but in most cases you have simply deleted the >> init_thread_copy() function even when it's not at all obvious (to me anyway) >> that that won't break the codec. > In most cases, just deleting init_thread_copy() is the right thing to > do. E.g. all decoders that do not implement update_thread_context() have > to be intra-only, so every frame thread is effectively a completely > standalone decoder. And in most of the more complex decoders (like h264) > the important parameters are dynamically changeable during decoding, so > not that much is done in decoder init beyond allocating some stuff that > does not depend on the bistream properties. > > My intent is for each frame-thread worker to be treated inasmuch as > possible as a standalone decoder, and where it has to share data with > other threads to make this sharing explicit (rather than implicit as is > the case now). Yes, this makes sense. The confusing part is when the decode_init() function looks completely different than the init_thread_copy() function. This is often because the decode_init() function is generating things (tables, etc.) from scratch and the init_thread_copy() is just copying the necessary things from the original. In cases where the original generation might be time consuming this might make sense, but usually it's probably just making the code more complex and difficult to follow (which I believe was your original point). One possible interim solution for complex cases that break would be to leave the init_thread_copy() function there, but instead of having it in the AVCodec struct and called from outside (which is no longer possible), it simply gets called first thing from decode_init() if AVCodecInternal.is_copy is set. That way the architecture is cleaned up now, and the codec won't break and can be cleaned up when time permits. Just a thought. > >> Specifically this will break WavPack because now it will be allocating a new >> dsdctx for every thread instead of sharing one >> between all threads. I am happy to fix and test this case once the patch >> goes in, but is the intent of this patch that the >> respective authors will find and fix all the breakages? Or did I just happen >> to catch the one case you missed? > I certainly intended to convert the decoders correctly myself, > apparently I didn't pay enough attention to the recent wavpack changes. > Hopefully the other conversions are correct, but I will go through the > changes again to check. > > Looking at wavpack, the code looks suspicious to me. You allocate one > dsdctx per channel at init, but AFAIU the number of channels can change > at each frame. > Multichannel WavPack consists of sequences of stereo or mono WavPack blocks that include flags indicating whether they are the beginning or end of a "frame" or "packet". This allows the number of channels available to be essentially unlimited, however the total number of channels in a stream may not change. When decoding, if a sequence of blocks generates more or less than the correct number of channels, this is flagged as an error and overrun is prevented (see libavcodec/wavpack.c, line 1499 and line 1621). If you are curious, the WavPack format is described in detail here: https://github.com/dbry/WavPack/blob/master/doc/WavPack5FileFormat.pdf Thanks! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
Quoting David Bryant (2020-03-27 23:51:19) > On 3/27/20 5:57 AM, Anton Khirnov wrote: > > The current design, where > > - proper init is called for the first per-thread context > > - first thread's private data is copied into private data for all the > > other threads > > - a "fixup" function is called for all the other threads to e.g. > > allocate dynamically allocated data > > is very fragile and hard to follow, so it is abandoned. Instead, the > > same init function is used to init each per-thread context. Where > > necessary, AVCodecInternal.is_copy can be used to differentiate between > > the first thread and the other ones (e.g. for decoding the extradata > > just once). > > --- > > I'm not sure I fully understand this change. You mention that > AVCodecInternal.is_copy can be used where different treatment > might be necessary for subsequent threads, and I see that done in a couple > places, but in most cases you have simply deleted the > init_thread_copy() function even when it's not at all obvious (to me anyway) > that that won't break the codec. In most cases, just deleting init_thread_copy() is the right thing to do. E.g. all decoders that do not implement update_thread_context() have to be intra-only, so every frame thread is effectively a completely standalone decoder. And in most of the more complex decoders (like h264) the important parameters are dynamically changeable during decoding, so not that much is done in decoder init beyond allocating some stuff that does not depend on the bistream properties. My intent is for each frame-thread worker to be treated inasmuch as possible as a standalone decoder, and where it has to share data with other threads to make this sharing explicit (rather than implicit as is the case now). > > Specifically this will break WavPack because now it will be allocating a new > dsdctx for every thread instead of sharing one > between all threads. I am happy to fix and test this case once the patch goes > in, but is the intent of this patch that the > respective authors will find and fix all the breakages? Or did I just happen > to catch the one case you missed? I certainly intended to convert the decoders correctly myself, apparently I didn't pay enough attention to the recent wavpack changes. Hopefully the other conversions are correct, but I will go through the changes again to check. Looking at wavpack, the code looks suspicious to me. You allocate one dsdctx per channel at init, but AFAIU the number of channels can change at each frame. -- Anton Khirnov ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/27/20 5:57 AM, Anton Khirnov wrote: > The current design, where > - proper init is called for the first per-thread context > - first thread's private data is copied into private data for all the > other threads > - a "fixup" function is called for all the other threads to e.g. > allocate dynamically allocated data > is very fragile and hard to follow, so it is abandoned. Instead, the > same init function is used to init each per-thread context. Where > necessary, AVCodecInternal.is_copy can be used to differentiate between > the first thread and the other ones (e.g. for decoding the extradata > just once). > --- I'm not sure I fully understand this change. You mention that AVCodecInternal.is_copy can be used where different treatment might be necessary for subsequent threads, and I see that done in a couple places, but in most cases you have simply deleted the init_thread_copy() function even when it's not at all obvious (to me anyway) that that won't break the codec. Specifically this will break WavPack because now it will be allocating a new dsdctx for every thread instead of sharing one between all threads. I am happy to fix and test this case once the patch goes in, but is the intent of this patch that the respective authors will find and fix all the breakages? Or did I just happen to catch the one case you missed? Cheers! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
