[soc] Questions on patch order, threads and review comments
Hi Stefano, I have a few questions on the order of patches and on some of your review comments from earlier:
Here is the working af_resample.c and associated Makefile and allfilters.c changes.
[...]
-#define AV_PERM_READ 0x01 ///< can read from the buffer -#define AV_PERM_WRITE 0x02 ///< can write to the buffer -#define AV_PERM_PRESERVE 0x04 ///< nobody else can overwrite the buffer -#define AV_PERM_REUSE 0x08 ///< can output the buffer multiple times, with the same contents each time -#define AV_PERM_REUSE2 0x10 ///< can output the buffer multiple times, modified each time
The AV_PERM_* moving can go to a separate patch.
Should I submit the AV_PERM_* patch first (move it out of AVFilterPicRef structure) and then the the audio framework changes patch? Or should I focus on the audio filter patches and define the PERM macros twice (inside AVFilterPicRef and AVFilterSamplesRef structures).
- link->format = link->in_formats->formats[0]; + if (link->type == AVMEDIA_TYPE_VIDEO) + link->format = link->in_formats->formats[0]; + else if (link->type == AVMEDIA_TYPE_AUDIO) + link->aformat = link->in_formats->aformats[0];
simpler: link->format = type == VIDEO ? link->in_formats->formats[0] : link->in_formats->aformats[0];
Here I need to assign to link->format in case of video and link->aformat in case of audio. How can I get this done using the ?: form above?
+#define CMP_AND_ADD(acount, bcount, afmt, bfmt, retfmt) { \ + for(i = 0; i < acount; i++) \ + for(j = 0; j < bcount; j++) \ + if(afmt[i] == bfmt[j]) \ + retfmt[k++] = afmt[i]; \ +} [...]
This refactoring is nice but please split it in a separate patch (and separate thread).
Again, should I submit the CMP_AND_ADD refactoring patch first before the audio filter patches? I should start a new thread for each patch - is that right? Also, I now have some patches based on your comments and some of the additions for channel layout conversion etc. Should I send these as patches of patches or send the entire patch each time starting from whatever is not yet in the soc svns? Patches of patches would be difficult to read while sending the entire patch again hides the new changes. Finally, Wikipedia says the downmix formulae for ac3 to stereo are: Left Only = Left (Front) + -3dB*Center + att*LeftSide Right Only = Right (Front) + -3dB*Center + att*RightSide where att = -3dB, -6dB or -9dB. The article has no references. Can I just go ahead and use this? It seems to work well for a 5.0 track I have with att = -9dB. Regards, Hemanth Regards, Hemanth
On date Tuesday 2010-07-06 02:45:09 -0700, S.N. Hemanth Meenakshisundaram encoded:
Hi Stefano,
I have a few questions on the order of patches and on some of your review comments from earlier:
Here is the working af_resample.c and associated Makefile and allfilters.c changes.
[...]
-#define AV_PERM_READ 0x01 ///< can read from the buffer -#define AV_PERM_WRITE 0x02 ///< can write to the buffer -#define AV_PERM_PRESERVE 0x04 ///< nobody else can overwrite the buffer -#define AV_PERM_REUSE 0x08 ///< can output the buffer multiple times, with the same contents each time -#define AV_PERM_REUSE2 0x10 ///< can output the buffer multiple times, modified each time
The AV_PERM_* moving can go to a separate patch.
Should I submit the AV_PERM_* patch first (move it out of AVFilterPicRef structure) and then the the audio framework changes patch? Or should I focus on the audio filter patches and define the PERM macros twice (inside AVFilterPicRef and AVFilterSamplesRef structures).
Possibly to a separate change chronologically done *before* the audio framework patch. The objective is to make the audio framework patch smaller and simplify review. Also the first one should be pretty easy and pretty fast to get applied. Note that with git that shouldn't be at all an issue, you can work out the patches in a branch, change their position, merge and split them using git rebase -i, and no-one is preventing you to send many patches in a branch, so they can start to be discussed even when the pre-requisite patches (like this one) have not yet been applied.
- link->format = link->in_formats->formats[0]; + if (link->type == AVMEDIA_TYPE_VIDEO) + link->format = link->in_formats->formats[0]; + else if (link->type == AVMEDIA_TYPE_AUDIO) + link->aformat = link->in_formats->aformats[0];
simpler: link->format = type == VIDEO ? link->in_formats->formats[0] : link->in_formats->aformats[0];
Here I need to assign to link->format in case of video and link->aformat in case of audio. How can I get this done using the ?: form above?
Forget about that, my suggestion was just wrong. And you can put them in a single line to help readability if you prefer it: if (link->type == AVMEDIA_TYPE_VIDEO) link->format = link->in_formats->formats[0]; else if (link->type == AVMEDIA_TYPE_AUDIO) link->aformat = link->in_formats->aformats[0];
+#define CMP_AND_ADD(acount, bcount, afmt, bfmt, retfmt) { \ + for(i = 0; i < acount; i++) \ + for(j = 0; j < bcount; j++) \ + if(afmt[i] == bfmt[j]) \ + retfmt[k++] = afmt[i]; \ +} [...]
This refactoring is nice but please split it in a separate patch (and separate thread).
Again, should I submit the CMP_AND_ADD refactoring patch first before the audio filter patches?
You can send them *before* or *togheter* as you see fit.
I should start a new thread for each patch - is that right?
Possibly, that help the reviewer/committer to understand if a *specific* patch has been already approved / committed, better than browse through all the mails in a super-long thread.
Also, I now have some patches based on your comments and some of the additions for channel layout conversion etc.
Should I send these as patches of patches or send the entire patch each time starting from whatever is not yet in the soc svns? Patches of patches would be difficult to read while sending the entire patch again hides the new changes.
Patches as issued by git should be fine to post, and the number in the patch help to clarify the dependencies of the patch.
Finally, Wikipedia says the downmix formulae for ac3 to stereo are:
Left Only = Left (Front) + -3dB*Center + att*LeftSide
Right Only = Right (Front) + -3dB*Center + att*RightSide
where att = -3dB, -6dB or -9dB.
The article has no references. Can I just go ahead and use this? It seems to work well for a 5.0 track I have with att = -9dB.
If it works and some audio-competent reviewer approve it should be safe, anyway a reference in the code to the original article would be nice / useful. Regards.
Hi All, With the framework up and af_resample working on the framework, what other kinds of audio filters can I work on? Stefano had earlier suggested exploring 1. Audio Visualization filters and 2. Digital filters applied to audio. I have a couple of questions about this: 1. What kinds of digital filters would prove useful? How about an equalizer for amplifying different parts of the spectrum? 2. For audio visualization systems, how can I access the video thread to display the visualizations. What kind of features are not present in the existing visualization? Thanks, Hemanth
On Fri, Jul 16, 2010 at 01:44:27AM -0700, S.N. Hemanth Meenakshisundaram wrote:
Hi All,
With the framework up and af_resample working on the framework, what other kinds of audio filters can I work on?
Stefano had earlier suggested exploring 1. Audio Visualization filters and 2. Digital filters applied to audio.
I have a couple of questions about this:
[...]
2. For audio visualization systems, how can I access the video thread to display the visualizations.
Ouch ! You don't want to access the video thread ! What you want is to create a simple filter which take one audio input stream and generate one video output stream. For now it would be nice to implement (copy?) the same visualization effects that the ones ffplay supports. Then the visualization effects code could be remove from ffplay, and it could be replace by building a filter graph which would split audio in two identical streams. One stream would be feeded to a visualization filter and the other one would just passthru to the end of the filter graph. Aurel
On 07/16/2010 06:53 AM, Aurelien Jacobs wrote:
On Fri, Jul 16, 2010 at 01:44:27AM -0700, S.N. Hemanth Meenakshisundaram wrote:
[...]
2. For audio visualization systems, how can I access the video thread to display the visualizations.
Ouch ! You don't want to access the video thread ! What you want is to create a simple filter which take one audio input stream and generate one video output stream. For now it would be nice to implement (copy?) the same visualization effects that the ones ffplay supports. Then the visualization effects code could be remove from ffplay, and it could be replace by building a filter graph which would split audio in two identical streams. One stream would be feeded to a visualization filter and the other one would just passthru to the end of the filter graph.
Hi All, This is what I am trying to write now: i/p audio filter --------> af-split ----------> o/p audio filter | ------------------> af_viz ----------> o/p visualization filter When af_viz (or equivalent) is specified as part of -af option, the framework sets up af_split and o/p viz filter. af_viz has an input pad of type audio and o/p pad of type video and will ouput video frames in YUV like any video filter. o/p visualization filter will manage the queue of frames from af_viz. A function replacing the video_audio_display function in ffplay.c will call request_frame on the o/p viz filter and prepare it for SDL or whatever else is used in the future for video output. One issue is the formats of the af_viz. For the moment, I am giving it the default query_formats (support all formats) behavior, so that when avfiltergraph checks for common formats, there are no issues. Error handling would have to be within the filter - i.e. if af_viz doesn't get audio in a format it supports, it should complain etc. Any comments on the design, suggestions? Regards,
Hi, On Fri, Jul 16, 2010 at 4:44 AM, S.N. Hemanth Meenakshisundaram <smeenaks@ucsd.edu> wrote:
1. What kinds of digital filters would prove useful? How about an equalizer for amplifying different parts of the spectrum?
The various audio encoders/decoders have a whole bunch of these that might be fun to expose: - denoising, low/high-pass (does this make sense to expose as a filter? I think it does) - equalizer would be cool - check ffaacenc for tons more Ronald
participants (4)
-
Aurelien Jacobs -
Ronald S. Bultje -
S.N. Hemanth Meenakshisundaram -
Stefano Sabatini