On 7/24/19 12:26 AM, Paul B Mahol wrote: > On 7/23/19, David Bryant <da...@wavpack.com> wrote: >> On 7/23/19 12:47 AM, Paul B Mahol wrote: >>> On 7/23/19, David Bryant <da...@wavpack.com> wrote: >>>> On 7/21/19 11:23 PM, Paul B Mahol wrote: >>>>> On 7/22/19, David Bryant <da...@wavpack.com> wrote: >>>>>> Hi, >>>>>> >>>>>> As I promised late last year, here is a patch to add a WavPack DSD >>>>>> decoder. >>>>>> >>>>>> Thanks! >>>>>> >>>>>> -David Bryant >>>>>> >>>>>> >>>>> Please correct me if I'm wrong, but why this uses new codec id? >>>>> Apparently is also copies some logic from other files. >>>> Yes, I created a new codec ID for WavPack DSD. It would be possible to >>>> just >>>> piggyback on the existing WavPack codec by silently >>>> decimating the DSD to PCM, but that seemed weird and wrong to me. For one >>>> thing, the user would have no idea that the file was >>>> actually DSD and not high sample-rate PCM. >>>> >>>> Also, since regular WavPack has threading enabled but WavPack DSD can't >>>> (because of the dsd2pcm conversion) I would have to turn >>>> off threading for all WavPack (unless there's some way of making that >>>> conditional at runtime). It would also mean that regular >>>> WavPack would be dependent on the dsd2pcm component even if DSD was not >>>> required (not everyone needs DSD). And of course I was >>>> looking closely at the only other DSD codec in FFmpeg (DST) which has its >>>> own codec ID. >>>> >>>> Because regular WavPack PCM and DSD share the same block format and >>>> metadata >>>> structure, there is a little code that is shared >>>> between the two codecs (although they are no longer identical because of >>>> the >>>> threading difference). Is this a problem? I could >>>> combine the two codecs into one file and sprinkle in a few conditionals, >>>> but >>>> I don't think it would be as clean or clear (but >>>> might save a little duplicate code). >>>> >>>> That's my thinking, but obviously I am not as familiar with the FFmpeg >>>> philosophy as you guys. >>> Looking carefully at dsd2pcm code in your patch, it appears it would >>> work only with 1 or 2 channels. >>> Is this correct? >> Individual WavPack blocks can only be 1 or 2 channels. Multichannel files >> contains sequences of blocks which ends up creating >> multiple frame contexts. This was originally implemented for PCM WavPack >> years ago and so it "just works" for DSD WavPack >> because I didn't touch any of that. >> >> I have tested this extensively with multichannel DSD files and it works >> great, including seeking. > Than I fail to see why it could not work for multi-threading too.
Really? Because I just looked back at the FFmpeg devel archives (May 2016) and you supplied the patch for the DST codec, the only other DSD compression codec in existence before my recent development. And dstdec.c does not enable multi-threading because, of course, you get clicks. This seems like an unlikely oversight since DST is a well-documented CPU hog and would benefit greatly from multi-threading. > >> >>> About multi-threading, as dsd2pcm can not be made multi-threaded then >>> just do locking when doing dsd2pcm to make it single threaded and >>> update other code as necessary. >> It's not really that dsd2pcm cannot be multi-threaded, it's that the audio >> frames cannot be converted to PCM independently or >> you'll get a click at the frame boundaries. Maybe we're talking about the >> same thing, but I don't see how a lock would fix this. >> In any event, could you please show me an example of doing that? > The ffmpeg have functionality to await progress of some work, which > allows doing some stuff single threaded. > For example look at apng decoder in libavcodec/pngdec.c > See ff_thread_await/report_progress and > .update_thread_context/.init_thread_copy > and relevant documentation of mentioned functions. Okay, I've looked over that stuff and I agree that it might be usable for this, although it is currently only used for video codecs and pictures, and it might be tricky getting it right (at least for someone not intimate with FFmpeg internals). What would be cool is if you could implement this in dstdec.c and I'll copy that... ;-) Or maybe I'll figure it out first and provide a patch to dstdec. > >> >>> I still see no valid reason to add separate codec id. Add another >>> wavpack profile if you wish to help users know the difference between >>> the two. >> Again I apologize for not being too familiar with FFmpeg internals. What >> would another wavpack "profile" be? As long as the user >> (or caller) has some way to differentiate between DSD and PCM files without >> parsing the file themselves I'm happy, but what >> exactly is the hesitation here? Are codec IDs a limited resource? It seems >> like an obvious win to me. > For profile explanation look how other codecs do it, like > libavcodec/proresdec2.c > Copy pasting other code is sure never win. Not sure what your last sentence means, but I'm assuming it means that copying other code is okay (which I agree with). I looked over the profile stuff and it will work for what I need uniquely identifying WavPack DSD files. However, in the other instances the word "profile" has the common meaning of being just a variant method of encoding the same data (like AAC LC vs. HE-AAC vs. LD), whereas in the WavPack DSD case we are talking about a completely different source data format and codec that just happen to be in the same container. But okay, if that's what I gotta do. Unfortunately I'm a little short on time and so I'm not sure when I can get to this refactoring. Therefore I have finalized the existing patch with all the other feedback so that if anyone wants to test or use it they can. I believe it is complete and safe. Thanks for your feedback. Kind regards, David > >> -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". >> >> _______________________________________________ >> 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". > _______________________________________________ > 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". _______________________________________________ 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".
