| Message ID | 20170722140232.962-1-mfcc64@gmail.com |
|---|---|
| State | New |
| Headers | show |
Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : > The behaviour is based on assumptions: > - copying audio data is cheap, so keeping data alignment is cheap, > - copying video data is not cheap, so keeping data alignment is not cheap, > e.g. crop filter. > > Should fix Ticket6349. > Note that after fc3a03fcf9cd7eafe7342e2508e6128888efa0bb, the crash has > been fixed. s/fixed/worked around in the most common situations/ > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavutil/frame.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavutil/frame.h b/libavutil/frame.h > index 26261d7e40..1b08eec231 100644 > --- a/libavutil/frame.h > +++ b/libavutil/frame.h > @@ -202,6 +202,9 @@ typedef struct AVFrame { > * up to 16 bytes beyond the planes, if these filters are to be used, > * then 16 extra bytes must be allocated. > * > + * For audio, the data should be aligned to CPUs alignment preference. I think this is a bad idea: it is an API change, and a rather bad one on top of that: - The "CPUs alignment preference" is not documented. - Requiring the application to take care of it when it could be done by the library is a waste of effort. > + * For video, no restriction on the alignment. > + * > * NOTE: Except for hwaccel formats, pointers not needed by the format > * MUST be set to NULL. > */ Regards,
On Sat, Jul 22, 2017 at 9:14 PM, Nicolas George <george@nsup.org> wrote: > Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : >> The behaviour is based on assumptions: >> - copying audio data is cheap, so keeping data alignment is cheap, >> - copying video data is not cheap, so keeping data alignment is not cheap, >> e.g. crop filter. >> >> Should fix Ticket6349. > >> Note that after fc3a03fcf9cd7eafe7342e2508e6128888efa0bb, the crash has >> been fixed. > > s/fixed/worked around in the most common situations/ It fixed the testcase's crash, but didn't fix the bug. > >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> --- >> libavutil/frame.h | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libavutil/frame.h b/libavutil/frame.h >> index 26261d7e40..1b08eec231 100644 >> --- a/libavutil/frame.h >> +++ b/libavutil/frame.h >> @@ -202,6 +202,9 @@ typedef struct AVFrame { >> * up to 16 bytes beyond the planes, if these filters are to be used, >> * then 16 extra bytes must be allocated. >> * > >> + * For audio, the data should be aligned to CPUs alignment preference. > > I think this is a bad idea: it is an API change, and a rather bad one on > top of that: > > - The "CPUs alignment preference" is not documented. It is documented on linesize documentation. > > - Requiring the application to take care of it when it could be done by > the library is a waste of effort. It is easy for application: - always use av_frame_get_buffer - never modify audio data pointer. On the contrary, handling unaligned data/linesize in the library is not easy. > >> + * For video, no restriction on the alignment. >> + * >> * NOTE: Except for hwaccel formats, pointers not needed by the format >> * MUST be set to NULL. >> */ > > Regards, > > -- > Nicolas George Thank's.
Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : > It fixed the testcase's crash, but didn't fix the bug. So it is not FIXED. > It is easy for application: > - always use av_frame_get_buffer > - never modify audio data pointer. Even easier: never use libavcodec. Kind of defeating the purpose. The point of a library is to reduce the amount of code required by applications. By adding that constraint, you either restrict what the applications can do with the library or require more code from them. Both are bad, both should be avoided if possible. And in this particular case it is possible. > On the contrary, handling unaligned data/linesize in the library is not easy. It is very easy, I have already implemented it. The patch is on the mailing-list since two months ago. Regards,
On Sat, Jul 22, 2017 at 9:53 PM, Nicolas George <george@nsup.org> wrote: > Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : >> It fixed the testcase's crash, but didn't fix the bug. > > So it is not FIXED. > >> It is easy for application: >> - always use av_frame_get_buffer >> - never modify audio data pointer. > > Even easier: never use libavcodec. Kind of defeating the purpose. The > point of a library is to reduce the amount of code required by > applications. By adding that constraint, you either restrict what the > applications can do with the library or require more code from them. > Both are bad, both should be avoided if possible. And in this particular > case it is possible. Of course, the linesize constraint has already been there. Should we relax this constraint? Also, generally FFmpeg's API is full of constraints. > >> On the contrary, handling unaligned data/linesize in the library is not easy. > > It is very easy, I have already implemented it. The patch is on the > mailing-list since two months ago. So, why did you stop working on this? I just want to take an easier path. Thank's.
On Sat, 22 Jul 2017 at 15:14 Nicolas George <george@nsup.org> wrote: > Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : > > The behaviour is based on assumptions: > > - copying audio data is cheap, so keeping data alignment is cheap, > > - copying video data is not cheap, so keeping data alignment is not > cheap, > > e.g. crop filter. > > > > Should fix Ticket6349. > > > Note that after fc3a03fcf9cd7eafe7342e2508e6128888efa0bb, the crash has > > been fixed. > > s/fixed/worked around in the most common situations/ > > > > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > > --- > > libavutil/frame.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/libavutil/frame.h b/libavutil/frame.h > > index 26261d7e40..1b08eec231 100644 > > --- a/libavutil/frame.h > > +++ b/libavutil/frame.h > > @@ -202,6 +202,9 @@ typedef struct AVFrame { > > * up to 16 bytes beyond the planes, if these filters are to be > used, > > * then 16 extra bytes must be allocated. > > * > > > + * For audio, the data should be aligned to CPUs alignment > preference. > > I think this is a bad idea: it is an API change, and a rather bad one on > top of that: > > - The "CPUs alignment preference" is not documented. > > - Requiring the application to take care of it when it could be done by > the library is a waste of effort. > In practice this is not an API change because it is required for some decoders in practice (Opus and AVX2 comes to mind). I sent a similar patch for this 3+ years ago and nothing was done with it. https://ffmpeg.org/pipermail/ffmpeg-devel/2014-August/160862.html Kieran
Le quartidi 4 thermidor, an CCXXV, Muhammad Faiz a écrit : > Of course, the linesize constraint has already been there. Should we > relax this constraint? Yes, if we can, we should. But I am not implying any kind of priority about it. Relaxing a constraint that is already present since long ago is not the same thing as adding a new constraint. > Also, generally FFmpeg's API is full of constraints. All APIs are full of constraints. It is a matter of balance between how hard it is for the application to meet the constraint versus how hard it is for the library to drop the constraint. > So, why did you stop working on this? Because there was too much of what I perceived as bikeshedding. But feel free to take over the patch: technically, it was finished, working, passing FATE, etc. It just needs pushing. > I just want to take an easier path. I realize that. But when API is concerned, taking the easier path for the developer comes second after trying to make the API more convenient. Regards,
Le quartidi 4 thermidor, an CCXXV, Kieran Kunhya a écrit : > In practice this is not an API change because it is required for some > decoders in practice (Opus and AVX2 comes to mind). Yes, but if that constraint is not documented then any crash should be considered a bug in FFmpeg, not a violation by the application. Thus, I stand by it, it is an API change. > I sent a similar patch for this 3+ years ago and nothing was done with it. > > https://ffmpeg.org/pipermail/ffmpeg-devel/2014-August/160862.html This was about get_buffer(), an advanced API for high efficiency. Also, all this was already rehashed three months ago. Let us not start again. Regards,
diff --git a/libavutil/frame.h b/libavutil/frame.h index 26261d7e40..1b08eec231 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -202,6 +202,9 @@ typedef struct AVFrame { * up to 16 bytes beyond the planes, if these filters are to be used, * then 16 extra bytes must be allocated. * + * For audio, the data should be aligned to CPUs alignment preference. + * For video, no restriction on the alignment. + * * NOTE: Except for hwaccel formats, pointers not needed by the format * MUST be set to NULL. */
The behaviour is based on assumptions: - copying audio data is cheap, so keeping data alignment is cheap, - copying video data is not cheap, so keeping data alignment is not cheap, e.g. crop filter. Should fix Ticket6349. Note that after fc3a03fcf9cd7eafe7342e2508e6128888efa0bb, the crash has been fixed. Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> --- libavutil/frame.h | 3 +++ 1 file changed, 3 insertions(+)