| Message ID | 20170506092006.22397-1-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, May 6, 2017 at 4:20 PM, Nicolas George <george@nsup.org> wrote: > TODO Actual implementation. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavutil/frame.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > > Here is a proposal for actually fixing the alignment problems that are all > over the place in the current code base. > > Note that it is only a design proposal, all the actual implementation is > missing. I do not want to waste my time implementing if the design is to be > bikeshedded to death. > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 4d8c1bed4f..162ea0716c 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -746,6 +746,15 @@ void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type); > const char *av_frame_side_data_name(enum AVFrameSideDataType type); > > /** > + * Realign the data pointers of a frame. > + * Make sure all frame data pointers have the alignment lower bits cleared, > + * i.e. are a multiple of 1<<alignment. > + * If not, a new frame is allocated and overwrites the current one. > + * @return >=0 for success or an AVERROR code. > + */ > +int av_frame_realign(AVFrame *frame, unsigned align); > + > +/** > * @} > */ > > -- > 2.11.0 Whatever the result here (accepted or rejected), we still need a fix without API change. Otherwise, it can't go to 3.3 branch. Thank's.
Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : > Whatever the result here (accepted or rejected), we still need a fix > without API change. Otherwise, it can't go to 3.3 branch. There is no API change as is (except for the extra function in lavu, but that is only an add, and can be worked around), that is one of the points of the design. (I will answer to the other mails later, it takes a little more time.) Regards,
On Sat, May 6, 2017 at 9:39 PM, Nicolas George <george@nsup.org> wrote: > Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >> Whatever the result here (accepted or rejected), we still need a fix >> without API change. Otherwise, it can't go to 3.3 branch. > > There is no API change as is (except for the extra function in lavu, but > that is only an add, and can be worked around), that is one of the > points of the design. As far as I know. No new features can go to release branch. Or maybe I'm wrong. Thank's.
On Sat, May 6, 2017 at 5:40 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Sat, May 6, 2017 at 9:39 PM, Nicolas George <george@nsup.org> wrote: >> Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >>> Whatever the result here (accepted or rejected), we still need a fix >>> without API change. Otherwise, it can't go to 3.3 branch. >> >> There is no API change as is (except for the extra function in lavu, but >> that is only an add, and can be worked around), that is one of the >> points of the design. > > As far as I know. No new features can go to release branch. > Or maybe I'm wrong. > This is correct. The 3.3 branch should be fixed without new API or some complex design, perhaps with the patch posted that re-aligns framequeue output. - Hendrik
Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : > As far as I know. No new features can go to release branch. > Or maybe I'm wrong. As I said, if this is the only issue, then it is not: just copy-paste av_frame_realign() at the two places where it will be needed and make it static. But really, I do not care much what happens in releases. I will fight to the end to prevent short-term workarounds to be introduced in the master branch when a correct fix is so easy, but for the releases branches, do what you want. Only do not expect my help for the ugly way. Regards,
On Sat, 6 May 2017, Nicolas George wrote: > Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >> As far as I know. No new features can go to release branch. >> Or maybe I'm wrong. > > As I said, if this is the only issue, then it is not: just copy-paste > av_frame_realign() at the two places where it will be needed and make it > static. > > But really, I do not care much what happens in releases. I will fight to > the end to prevent short-term workarounds to be introduced in the master > branch when a correct fix is so easy, but for the releases branches, do > what you want. Only do not expect my help for the ugly way. I suggest the following: As the av_frame_realign API is useful on its own without any framework, I suggest we implement that. Once the implementation is complete we can backport it to 3.3 with the avpriv prefix and without bumping version numbers. In the 3.3 branch the regression can be fixed (old behaviour can be restored) by calling avpriv_frame_align in the frame queue. In the master branch, if no consensus is reached, or discussion/work on the alignment framework stalls, a vote can be made to either 1) enforce the aligment in the frame queue or 2) enforce the aligment in parts of the code we know regressed (libmp3, af_volume) Regards, Marton
On Sat, 6 May 2017, Nicolas George wrote: > TODO Actual implementation. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavutil/frame.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > > Here is a proposal for actually fixing the alignment problems that are all > over the place in the current code base. > > Note that it is only a design proposal, all the actual implementation is > missing. I do not want to waste my time implementing if the design is to be > bikeshedded to death. > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 4d8c1bed4f..162ea0716c 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -746,6 +746,15 @@ void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type); > const char *av_frame_side_data_name(enum AVFrameSideDataType type); > > /** > + * Realign the data pointers of a frame. > + * Make sure all frame data pointers have the alignment lower bits cleared, > + * i.e. are a multiple of 1<<alignment. > + * If not, a new frame is allocated and overwrites the current one. > + * @return >=0 for success or an AVERROR code. I suggest to return 1 if an actual alignment (with deep copy) happened. > + */ > +int av_frame_realign(AVFrame *frame, unsigned align); > + > +/** > * @} > */ Regards, Marton
Hi, On Sun, May 7, 2017 at 7:34 AM, Marton Balint <cus@passwd.hu> wrote: > > On Sat, 6 May 2017, Nicolas George wrote: > > Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >> >>> As far as I know. No new features can go to release branch. >>> Or maybe I'm wrong. >>> >> >> As I said, if this is the only issue, then it is not: just copy-paste >> av_frame_realign() at the two places where it will be needed and make it >> static. >> >> But really, I do not care much what happens in releases. I will fight to >> the end to prevent short-term workarounds to be introduced in the master >> branch when a correct fix is so easy, but for the releases branches, do >> what you want. Only do not expect my help for the ugly way. >> > > I suggest the following: > > As the av_frame_realign API is useful on its own without any framework, I > suggest we implement that. > > Once the implementation is complete we can backport it to 3.3 with the > avpriv prefix and without bumping version numbers. > > In the 3.3 branch the regression can be fixed (old behaviour can be > restored) by calling avpriv_frame_align in the frame queue. > > In the master branch, if no consensus is reached, or discussion/work on > the alignment framework stalls, a vote can be made to either > 1) enforce the aligment in the frame queue or > 2) enforce the aligment in parts of the code we know regressed (libmp3, > af_volume) My understanding from what Nicolas said is that the goal is to have higher-level code (utils.c-level, e.g. the callers of filter() or encode_frame() or get_buffer() inside decode_frame()) call this for every input frame going back to a caller with higher alignment requirements than the input frame (possibly with a one-time warning)? Ronald
L'octidi 18 floréal, an CCXXV, Ronald S. Bultje a écrit : > My understanding from what Nicolas said is that the goal is to have > higher-level code (utils.c-level, e.g. the callers of filter() or > encode_frame() or get_buffer() inside decode_frame()) call this for every > input frame going back to a caller with higher alignment requirements than ^^^^^^ > the input frame (possibly with a one-time warning)? I think you should have written "callee" here. Apart from that, this is exactly that, thanks. Regards,
L'octidi 18 floréal, an CCXXV, Marton Balint a écrit : > As the av_frame_realign API is useful on its own without any framework, I > suggest we implement that. > > Once the implementation is complete we can backport it to 3.3 with the > avpriv prefix and without bumping version numbers. > > In the 3.3 branch the regression can be fixed (old behaviour can be > restored) by calling avpriv_frame_align in the frame queue. You mean do that even before the discussion about the complete fix in master is finished? I can live with that. A small nit: since the avpriv_frame_realign() would be called from only one file, at most two, it would be simpler and cleaner to put it there as a static function than adding yet another function in the avpriv namespace. Also, depending on the time it takes for the discussion to finish (IIRC, only Hendrik had an objection to the principle), it may be more convenient to keep the branch as close as possible to master. > In the master branch, if no consensus is reached, or discussion/work on the > alignment framework stalls, a vote can be made to either > 1) enforce the aligment in the frame queue or > 2) enforce the aligment in parts of the code we know regressed (libmp3, > af_volume) Ronald explained, I will do so again with different words to make the point: My plan is to call av_frame_realign() in libavcodec/encode.c just before the calls to encoder->encode2() and encoder->send_frame() because these are the functions that need alignment. In other words, call it in the last place of the code path that is common to all encoders. For filters, I think the correct place is before the end of ff_inlink_consume_frame() and ff_inlink_consume_samples(). Regards,
On Sun, May 7, 2017 at 6:34 PM, Marton Balint <cus@passwd.hu> wrote: > > On Sat, 6 May 2017, Nicolas George wrote: > >> Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >>> >>> As far as I know. No new features can go to release branch. >>> Or maybe I'm wrong. >> >> >> As I said, if this is the only issue, then it is not: just copy-paste >> av_frame_realign() at the two places where it will be needed and make it >> static. >> >> But really, I do not care much what happens in releases. I will fight to >> the end to prevent short-term workarounds to be introduced in the master >> branch when a correct fix is so easy, but for the releases branches, do >> what you want. Only do not expect my help for the ugly way. > > > I suggest the following: > > As the av_frame_realign API is useful on its own without any framework, I > suggest we implement that. > > Once the implementation is complete we can backport it to 3.3 with the > avpriv prefix and without bumping version numbers. > > In the 3.3 branch the regression can be fixed (old behaviour can be > restored) by calling avpriv_frame_align in the frame queue. Inside libavfilter, we need something like ff_inlink_frame_align because of ff_get_buffer problem. This is fortunate because we don't need to call av_frame_realign at all. > > In the master branch, if no consensus is reached, or discussion/work on the > alignment framework stalls, a vote can be made to either > 1) enforce the aligment in the frame queue or > 2) enforce the aligment in parts of the code we know regressed (libmp3, > af_volume)
On Sun, May 7, 2017 at 1:34 PM, Marton Balint <cus@passwd.hu> wrote: > > On Sat, 6 May 2017, Nicolas George wrote: > >> Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >>> >>> As far as I know. No new features can go to release branch. >>> Or maybe I'm wrong. >> >> >> As I said, if this is the only issue, then it is not: just copy-paste >> av_frame_realign() at the two places where it will be needed and make it >> static. >> >> But really, I do not care much what happens in releases. I will fight to >> the end to prevent short-term workarounds to be introduced in the master >> branch when a correct fix is so easy, but for the releases branches, do >> what you want. Only do not expect my help for the ugly way. > > > I suggest the following: > > As the av_frame_realign API is useful on its own without any framework, I > suggest we implement that. > > Once the implementation is complete we can backport it to 3.3 with the > avpriv prefix and without bumping version numbers. > > In the 3.3 branch the regression can be fixed (old behaviour can be > restored) by calling avpriv_frame_align in the frame queue. > > In the master branch, if no consensus is reached, or discussion/work on the > alignment framework stalls, a vote can be made to either > 1) enforce the aligment in the frame queue or > 2) enforce the aligment in parts of the code we know regressed (libmp3, > af_volume) > Introducing avpriv API is still a bad thing for a release branch - avpriv is ultimately public ABI and having 3.3 and master diverge in ABI right away would be bad. IMHO just add a fix local to avfilter and make it re-align the frame queue to resolve the regression at hand in the release. - Hendrik
On Sun, May 7, 2017 at 1:47 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sun, May 7, 2017 at 7:34 AM, Marton Balint <cus@passwd.hu> wrote: > >> >> On Sat, 6 May 2017, Nicolas George wrote: >> >> Le septidi 17 floréal, an CCXXV, Muhammad Faiz a écrit : >>> >>>> As far as I know. No new features can go to release branch. >>>> Or maybe I'm wrong. >>>> >>> >>> As I said, if this is the only issue, then it is not: just copy-paste >>> av_frame_realign() at the two places where it will be needed and make it >>> static. >>> >>> But really, I do not care much what happens in releases. I will fight to >>> the end to prevent short-term workarounds to be introduced in the master >>> branch when a correct fix is so easy, but for the releases branches, do >>> what you want. Only do not expect my help for the ugly way. >>> >> >> I suggest the following: >> >> As the av_frame_realign API is useful on its own without any framework, I >> suggest we implement that. >> >> Once the implementation is complete we can backport it to 3.3 with the >> avpriv prefix and without bumping version numbers. >> >> In the 3.3 branch the regression can be fixed (old behaviour can be >> restored) by calling avpriv_frame_align in the frame queue. >> >> In the master branch, if no consensus is reached, or discussion/work on >> the alignment framework stalls, a vote can be made to either >> 1) enforce the aligment in the frame queue or >> 2) enforce the aligment in parts of the code we know regressed (libmp3, >> af_volume) > > > My understanding from what Nicolas said is that the goal is to have > higher-level code (utils.c-level, e.g. the callers of filter() or > encode_frame() or get_buffer() inside decode_frame()) call this for every > input frame going back to a caller with higher alignment requirements than > the input frame (possibly with a one-time warning)? > Just for the record, we can't do this with get_buffer, the entire point of this callback is to get a fully user-controlled buffer into a decoder. Anyone implementing this function needs to be absolutely aware of alignment and stride requirements, and for this function it is even documented already (although docs can always be expanded to make everything clearer). This entire concept currently applies to raw input data only, ie. input to avfilter, encoders, and maybe even swscale and swresample (although those may already have checks somewhere) - Hendrik
Hi, On Sun, May 7, 2017 at 7:50 AM, Nicolas George <george@nsup.org> wrote: > L'octidi 18 floréal, an CCXXV, Ronald S. Bultje a écrit : > > My understanding from what Nicolas said is that the goal is to have > > higher-level code (utils.c-level, e.g. the callers of filter() or > > encode_frame() or get_buffer() inside decode_frame()) call this for every > > input frame going back to a caller with higher alignment requirements > than > ^^^^^^ > > the input frame (possibly with a one-time warning)? > > I think you should have written "callee" here. Indeed, my bad. Apart from that, this is exactly that, thanks. OK, I think I agree with that design in principle. Hendrik is also right that it wouldn't be called after the get_buffer() user callback to _make_ the buffer aligned, but rather just to check that the buffer _is_ aligned. (If it isn't aligned, we would probably return an error from get_buffer.) Ronald
diff --git a/libavutil/frame.h b/libavutil/frame.h index 4d8c1bed4f..162ea0716c 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -746,6 +746,15 @@ void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type); const char *av_frame_side_data_name(enum AVFrameSideDataType type); /** + * Realign the data pointers of a frame. + * Make sure all frame data pointers have the alignment lower bits cleared, + * i.e. are a multiple of 1<<alignment. + * If not, a new frame is allocated and overwrites the current one. + * @return >=0 for success or an AVERROR code. + */ +int av_frame_realign(AVFrame *frame, unsigned align); + +/** * @} */
TODO Actual implementation. Signed-off-by: Nicolas George <george@nsup.org> --- libavutil/frame.h | 9 +++++++++ 1 file changed, 9 insertions(+) Here is a proposal for actually fixing the alignment problems that are all over the place in the current code base. Note that it is only a design proposal, all the actual implementation is missing. I do not want to waste my time implementing if the design is to be bikeshedded to death.