| Message ID | 20170426101605.7014-1-mfcc64@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 26 Apr 2017 17:16:05 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > This should fix Ticket6349 > > Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may > generate unaligned frame data. > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavcodec/libmp3lame.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > index 5e26743..cd505bb 100644 > --- a/libavcodec/libmp3lame.c > +++ b/libavcodec/libmp3lame.c > @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); > break; > case AV_SAMPLE_FMT_FLTP: > - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { > - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); > - return AVERROR(EINVAL); > - } > for (ch = 0; ch < avctx->channels; ch++) { > + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { > + float *src = (float *) frame->data[ch]; > + float *dst = s->samples_flt[ch]; > + int k; > + > + for (k = 0; k < frame->nb_samples; k++) > + dst[k] = src[k] * 32768.0f; > + } else { > s->fdsp->vector_fmul_scalar(s->samples_flt[ch], > (const float *)frame->data[ch], > 32768.0f, > FFALIGN(frame->nb_samples, 8)); > + } > } > ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); > break; Looks like the right solution is fixing framequeue. I don't think encoders generally allow lax alignment, and this might not be the only case.
On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Wed, 26 Apr 2017 17:16:05 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> This should fix Ticket6349 >> >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may >> generate unaligned frame data. >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> --- >> libavcodec/libmp3lame.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c >> index 5e26743..cd505bb 100644 >> --- a/libavcodec/libmp3lame.c >> +++ b/libavcodec/libmp3lame.c >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, >> ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); >> break; >> case AV_SAMPLE_FMT_FLTP: >> - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { >> - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); >> - return AVERROR(EINVAL); >> - } >> for (ch = 0; ch < avctx->channels; ch++) { >> + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { >> + float *src = (float *) frame->data[ch]; >> + float *dst = s->samples_flt[ch]; >> + int k; >> + >> + for (k = 0; k < frame->nb_samples; k++) >> + dst[k] = src[k] * 32768.0f; >> + } else { >> s->fdsp->vector_fmul_scalar(s->samples_flt[ch], >> (const float *)frame->data[ch], >> 32768.0f, >> FFALIGN(frame->nb_samples, 8)); >> + } >> } >> ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); >> break; > > Looks like the right solution is fixing framequeue. > > I don't think encoders generally allow lax alignment, and this might > not be the only case. This requirement is not documented. Also some filters may also generate unaligned data, for example crop filter.
On Wed, Apr 26, 2017 at 05:16:05PM +0700, Muhammad Faiz wrote: > This should fix Ticket6349 > > Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may > generate unaligned frame data. > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavcodec/libmp3lame.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > index 5e26743..cd505bb 100644 > --- a/libavcodec/libmp3lame.c > +++ b/libavcodec/libmp3lame.c > @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); > break; > case AV_SAMPLE_FMT_FLTP: > - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { > - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); > - return AVERROR(EINVAL); > - } > for (ch = 0; ch < avctx->channels; ch++) { > + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { > + float *src = (float *) frame->data[ch]; > + float *dst = s->samples_flt[ch]; > + int k; > + > + for (k = 0; k < frame->nb_samples; k++) > + dst[k] = src[k] * 32768.0f; If this solution is choosen then this should produce a one time log message to inform about the sub optimal alignment [...]
On Wed, 26 Apr 2017 20:09:58 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote: > > On Wed, 26 Apr 2017 17:16:05 +0700 > > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > >> This should fix Ticket6349 > >> > >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may > >> generate unaligned frame data. > >> > >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > >> --- > >> libavcodec/libmp3lame.c | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > >> index 5e26743..cd505bb 100644 > >> --- a/libavcodec/libmp3lame.c > >> +++ b/libavcodec/libmp3lame.c > >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > >> ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); > >> break; > >> case AV_SAMPLE_FMT_FLTP: > >> - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { > >> - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); > >> - return AVERROR(EINVAL); > >> - } > >> for (ch = 0; ch < avctx->channels; ch++) { > >> + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { > >> + float *src = (float *) frame->data[ch]; > >> + float *dst = s->samples_flt[ch]; > >> + int k; > >> + > >> + for (k = 0; k < frame->nb_samples; k++) > >> + dst[k] = src[k] * 32768.0f; > >> + } else { > >> s->fdsp->vector_fmul_scalar(s->samples_flt[ch], > >> (const float *)frame->data[ch], > >> 32768.0f, > >> FFALIGN(frame->nb_samples, 8)); > >> + } > >> } > >> ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); > >> break; > > > > Looks like the right solution is fixing framequeue. > > > > I don't think encoders generally allow lax alignment, and this might > > not be the only case. > > This requirement is not documented. Also some filters may also > generate unaligned data, for example crop filter. No - and it's inconsistently handled. But in general, everything seems to prefer aligned data, and blows up or becomes slow otherwise. Keep in mind that all data is allocated with large alignment in the first place. The crop filter is sort of a special case too.
On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Wed, 26 Apr 2017 20:09:58 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote: >> > On Wed, 26 Apr 2017 17:16:05 +0700 >> > Muhammad Faiz <mfcc64@gmail.com> wrote: >> > >> >> This should fix Ticket6349 >> >> >> >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may >> >> generate unaligned frame data. >> >> >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> >> --- >> >> libavcodec/libmp3lame.c | 13 +++++++++---- >> >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c >> >> index 5e26743..cd505bb 100644 >> >> --- a/libavcodec/libmp3lame.c >> >> +++ b/libavcodec/libmp3lame.c >> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, >> >> ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); >> >> break; >> >> case AV_SAMPLE_FMT_FLTP: >> >> - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { >> >> - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); >> >> - return AVERROR(EINVAL); >> >> - } >> >> for (ch = 0; ch < avctx->channels; ch++) { >> >> + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { >> >> + float *src = (float *) frame->data[ch]; >> >> + float *dst = s->samples_flt[ch]; >> >> + int k; >> >> + >> >> + for (k = 0; k < frame->nb_samples; k++) >> >> + dst[k] = src[k] * 32768.0f; >> >> + } else { >> >> s->fdsp->vector_fmul_scalar(s->samples_flt[ch], >> >> (const float *)frame->data[ch], >> >> 32768.0f, >> >> FFALIGN(frame->nb_samples, 8)); >> >> + } >> >> } >> >> ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); >> >> break; >> > >> > Looks like the right solution is fixing framequeue. >> > >> > I don't think encoders generally allow lax alignment, and this might >> > not be the only case. >> >> This requirement is not documented. Also some filters may also >> generate unaligned data, for example crop filter. > > No - and it's inconsistently handled. But in general, everything seems > to prefer aligned data, and blows up or becomes slow otherwise. Keep in > mind that all data is allocated with large alignment in the first place. Based on looking the code, it seems that volume filter cannot handle unaligned data, probably others too. Having this restriction without documenting it is considered bug. > > The crop filter is sort of a special case too. As consequence, video filters/codecs can handle it consistently. Note that frame.h only states that linesize should be multiple of 16/32. It does not state about data pointer. And crop filter does not modify linesize, and generating unaligned data pointer is legitimate.
On Thu, 27 Apr 2017 01:41:04 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote: > > On Wed, 26 Apr 2017 20:09:58 +0700 > > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote: > >> > On Wed, 26 Apr 2017 17:16:05 +0700 > >> > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> > > >> >> This should fix Ticket6349 > >> >> > >> >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may > >> >> generate unaligned frame data. > >> >> > >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > >> >> --- > >> >> libavcodec/libmp3lame.c | 13 +++++++++---- > >> >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> >> > >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > >> >> index 5e26743..cd505bb 100644 > >> >> --- a/libavcodec/libmp3lame.c > >> >> +++ b/libavcodec/libmp3lame.c > >> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > >> >> ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); > >> >> break; > >> >> case AV_SAMPLE_FMT_FLTP: > >> >> - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { > >> >> - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); > >> >> - return AVERROR(EINVAL); > >> >> - } > >> >> for (ch = 0; ch < avctx->channels; ch++) { > >> >> + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { > >> >> + float *src = (float *) frame->data[ch]; > >> >> + float *dst = s->samples_flt[ch]; > >> >> + int k; > >> >> + > >> >> + for (k = 0; k < frame->nb_samples; k++) > >> >> + dst[k] = src[k] * 32768.0f; > >> >> + } else { > >> >> s->fdsp->vector_fmul_scalar(s->samples_flt[ch], > >> >> (const float *)frame->data[ch], > >> >> 32768.0f, > >> >> FFALIGN(frame->nb_samples, 8)); > >> >> + } > >> >> } > >> >> ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); > >> >> break; > >> > > >> > Looks like the right solution is fixing framequeue. > >> > > >> > I don't think encoders generally allow lax alignment, and this might > >> > not be the only case. > >> > >> This requirement is not documented. Also some filters may also > >> generate unaligned data, for example crop filter. > > > > No - and it's inconsistently handled. But in general, everything seems > > to prefer aligned data, and blows up or becomes slow otherwise. Keep in > > mind that all data is allocated with large alignment in the first place. > > Based on looking the code, it seems that volume filter cannot handle > unaligned data, probably others too. > Having this restriction without documenting it is considered bug. > > > > > The crop filter is sort of a special case too. > > As consequence, video filters/codecs can handle it consistently. > Note that frame.h only states that linesize should be multiple of > 16/32. It does not state about data pointer. And crop filter does not > modify linesize, and generating unaligned data pointer is legitimate. Well yes, these things are not entirely documented. As a user I'd actually expect unaligned things to work, even if there's a performance impact. But still: - framequeue should probably not trigger low-performance code paths intentionally - it's hard to fix all the cases where unaligned things cause crashes AVPacket actually documented an alignment and padding requirement. I suppose AVFrame is less-well documented, because it has many use-cases associated (encoding, filtering, as direct rendering buffer for decoding, etc.). Where should we go from here? Personally I'd claim framequeue should be changed to produce aligned output. For encoders and filter input, we should probably add a flag whether unaligned input is supported, and if not, have the generic code before it copy the frame to make it aligned. I feel uneasy about that inconsistency being introduced by the framequeue thing, and adding another inconsistency by only fixing mp3lame... but maybe I'm making everything too complicated again.
On Wed, Apr 26, 2017 at 10:05:40PM +0200, wm4 wrote: > On Thu, 27 Apr 2017 01:41:04 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > On Wed, Apr 26, 2017 at 8:53 PM, wm4 <nfxjfg@googlemail.com> wrote: > > > On Wed, 26 Apr 2017 20:09:58 +0700 > > > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > > > >> On Wed, Apr 26, 2017 at 5:34 PM, wm4 <nfxjfg@googlemail.com> wrote: > > >> > On Wed, 26 Apr 2017 17:16:05 +0700 > > >> > Muhammad Faiz <mfcc64@gmail.com> wrote: > > >> > > > >> >> This should fix Ticket6349 > > >> >> > > >> >> Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may > > >> >> generate unaligned frame data. > > >> >> > > >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > > >> >> --- > > >> >> libavcodec/libmp3lame.c | 13 +++++++++---- > > >> >> 1 file changed, 9 insertions(+), 4 deletions(-) > > >> >> > > >> >> diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c > > >> >> index 5e26743..cd505bb 100644 > > >> >> --- a/libavcodec/libmp3lame.c > > >> >> +++ b/libavcodec/libmp3lame.c > > >> >> @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, > > >> >> ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); > > >> >> break; > > >> >> case AV_SAMPLE_FMT_FLTP: > > >> >> - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { > > >> >> - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); > > >> >> - return AVERROR(EINVAL); > > >> >> - } > > >> >> for (ch = 0; ch < avctx->channels; ch++) { > > >> >> + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { > > >> >> + float *src = (float *) frame->data[ch]; > > >> >> + float *dst = s->samples_flt[ch]; > > >> >> + int k; > > >> >> + > > >> >> + for (k = 0; k < frame->nb_samples; k++) > > >> >> + dst[k] = src[k] * 32768.0f; > > >> >> + } else { > > >> >> s->fdsp->vector_fmul_scalar(s->samples_flt[ch], > > >> >> (const float *)frame->data[ch], > > >> >> 32768.0f, > > >> >> FFALIGN(frame->nb_samples, 8)); > > >> >> + } > > >> >> } > > >> >> ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); > > >> >> break; > > >> > > > >> > Looks like the right solution is fixing framequeue. > > >> > > > >> > I don't think encoders generally allow lax alignment, and this might > > >> > not be the only case. > > >> > > >> This requirement is not documented. Also some filters may also > > >> generate unaligned data, for example crop filter. > > > > > > No - and it's inconsistently handled. But in general, everything seems > > > to prefer aligned data, and blows up or becomes slow otherwise. Keep in > > > mind that all data is allocated with large alignment in the first place. > > > > Based on looking the code, it seems that volume filter cannot handle > > unaligned data, probably others too. > > Having this restriction without documenting it is considered bug. > > > > > > > > The crop filter is sort of a special case too. > > > > As consequence, video filters/codecs can handle it consistently. > > Note that frame.h only states that linesize should be multiple of > > 16/32. It does not state about data pointer. And crop filter does not > > modify linesize, and generating unaligned data pointer is legitimate. > > Well yes, these things are not entirely documented. As a user I'd > actually expect unaligned things to work, even if there's a performance > impact. > > But still: > - framequeue should probably not trigger low-performance code paths > intentionally > - it's hard to fix all the cases where unaligned things cause crashes > > AVPacket actually documented an alignment and padding requirement. I > suppose AVFrame is less-well documented, because it has many use-cases > associated (encoding, filtering, as direct rendering buffer for > decoding, etc.). > > Where should we go from here? Personally I'd claim framequeue should be > changed to produce aligned output. > For encoders and filter input, we > should probably add a flag whether unaligned input is supported, and if > not, have the generic code before it copy the frame to make it aligned. I agree in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db) within the API of that time to avfilter [...]
L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit : > I agree > in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db) > within the API of that time to avfilter It was not a bad idea, but it should not be limited to filters. A few comments. * First, the framequeue framework does not produce unaligned code. According to the C standard, the data it handles stay aligned. The alignment problems come from non-standard requirements by special processor features used by some filters and codecs, but not all. * That means a lot of the most useful codecs and filters will suffer from it, but not all. For many tasks, the alignment is just fine, and the extra copy would be wasteful. * The alignment requirements increase. Before AVX, it was up to 16, now it can be 32, and I have no doubt future processor will at some point require 64 or 128. But realigning buffers used with SSE to 32 would be wasteful too. Thus, we do not require a flag but a full integer. * The code that does the actual work of realigning a buffer should available as a stand-alone API, to be used by applications that work at low-level. I suppose something like that would be in order: int av_frame_realign(AVFrame *frame, unsigned align); Or maybe: int av_frame_realign(AVFrame *frame, unsigned align, AVBufferAllocator *alloc); where AVBufferAllocator is there to allocate possibly hardware or mmaped buffers. * It is another argument for my leitmotiv that filters and codecs are actually the same and should be merged API-wise. * It would be better to have the API just work for everything rather than documenting the alignment needs. As for the actual implementation, I see a lot of different approaches: - have the framework realing the frame before submitting it to the filters and codecs: costly in malloc() and memcpy() but simple; - have each filter or codec call av_frame_realign() as needed; it may seem less elegant than the previous proposal, but it may prove a better choice in the light of what follows; - have each filter or codec copy the unaligned data into a buffer allocated once and for all or on the stack, possibly by small chunks: less costly in malloc() and refcounting overhead, and possibly better cache-locality, but more complex code; - run the plain C version of the code on unaligned data rather than the vectorized version, or the less-vectorized version (SSE vs AVX) on insufficiently aligned data. Since all this boils down to a matter of performance and is related to the core task of FFmpeg, I think the choice between the various options should be done on a case-by-case basis using real benchmarks. Regards,
On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@nsup.org> wrote: > L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit : >> I agree >> in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db) >> within the API of that time to avfilter > > It was not a bad idea, but it should not be limited to filters. A few > comments. > > * First, the framequeue framework does not produce unaligned code. > According to the C standard, the data it handles stay aligned. The > alignment problems come from non-standard requirements by special > processor features used by some filters and codecs, but not all. > > * That means a lot of the most useful codecs and filters will suffer > from it, but not all. For many tasks, the alignment is just fine, and > the extra copy would be wasteful. > > * The alignment requirements increase. Before AVX, it was up to 16, now > it can be 32, and I have no doubt future processor will at some point > require 64 or 128. But realigning buffers used with SSE to 32 would be > wasteful too. Thus, we do not require a flag but a full integer. > > * The code that does the actual work of realigning a buffer should > available as a stand-alone API, to be used by applications that work at > low-level. I suppose something like that would be in order: > > int av_frame_realign(AVFrame *frame, unsigned align); > > Or maybe: > > int av_frame_realign(AVFrame *frame, unsigned align, > AVBufferAllocator *alloc); > > where AVBufferAllocator is there to allocate possibly hardware or mmaped > buffers. > > * It is another argument for my leitmotiv that filters and codecs are > actually the same and should be merged API-wise. > > * It would be better to have the API just work for everything rather > than documenting the alignment needs. > > As for the actual implementation, I see a lot of different approaches: > > - have the framework realing the frame before submitting it to the > filters and codecs: costly in malloc() and memcpy() but simple; > > - have each filter or codec call av_frame_realign() as needed; it may > seem less elegant than the previous proposal, but it may prove a > better choice in the light of what follows; > > - have each filter or codec copy the unaligned data into a buffer > allocated once and for all or on the stack, possibly by small chunks: > less costly in malloc() and refcounting overhead, and possibly better > cache-locality, but more complex code; > > - run the plain C version of the code on unaligned data rather than the > vectorized version, or the less-vectorized version (SSE vs AVX) on > insufficiently aligned data. > > Since all this boils down to a matter of performance and is related to > the core task of FFmpeg, I think the choice between the various options > should be done on a case-by-case basis using real benchmarks. Are you working on these? Because currently I'm not. Thank's
Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit :
> Are you working on these? Because currently I'm not.
There is nothing to work on yet: the message you answer to is raising a
question about the global design of the internal API. That question
needs an answer before any work can be done, and I can not decide alone.
Regards,
On Sun, 30 Apr 2017 23:55:02 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@nsup.org> wrote: > > L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit : > >> I agree > >> in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db) > >> within the API of that time to avfilter > > > > It was not a bad idea, but it should not be limited to filters. A few > > comments. > > > > * First, the framequeue framework does not produce unaligned code. > > According to the C standard, the data it handles stay aligned. The > > alignment problems come from non-standard requirements by special > > processor features used by some filters and codecs, but not all. > > > > * That means a lot of the most useful codecs and filters will suffer > > from it, but not all. For many tasks, the alignment is just fine, and > > the extra copy would be wasteful. > > > > * The alignment requirements increase. Before AVX, it was up to 16, now > > it can be 32, and I have no doubt future processor will at some point > > require 64 or 128. But realigning buffers used with SSE to 32 would be > > wasteful too. Thus, we do not require a flag but a full integer. > > > > * The code that does the actual work of realigning a buffer should > > available as a stand-alone API, to be used by applications that work at > > low-level. I suppose something like that would be in order: > > > > int av_frame_realign(AVFrame *frame, unsigned align); > > > > Or maybe: > > > > int av_frame_realign(AVFrame *frame, unsigned align, > > AVBufferAllocator *alloc); > > > > where AVBufferAllocator is there to allocate possibly hardware or mmaped > > buffers. > > > > * It is another argument for my leitmotiv that filters and codecs are > > actually the same and should be merged API-wise. > > > > * It would be better to have the API just work for everything rather > > than documenting the alignment needs. > > > > As for the actual implementation, I see a lot of different approaches: > > > > - have the framework realing the frame before submitting it to the > > filters and codecs: costly in malloc() and memcpy() but simple; > > > > - have each filter or codec call av_frame_realign() as needed; it may > > seem less elegant than the previous proposal, but it may prove a > > better choice in the light of what follows; > > > > - have each filter or codec copy the unaligned data into a buffer > > allocated once and for all or on the stack, possibly by small chunks: > > less costly in malloc() and refcounting overhead, and possibly better > > cache-locality, but more complex code; > > > > - run the plain C version of the code on unaligned data rather than the > > vectorized version, or the less-vectorized version (SSE vs AVX) on > > insufficiently aligned data. > > > > Since all this boils down to a matter of performance and is related to > > the core task of FFmpeg, I think the choice between the various options > > should be done on a case-by-case basis using real benchmarks. > > Are you working on these? Because currently I'm not. I think it's probably ok to push your current patch, since the original author of the patch who broke this probably won't fix it.
On Mon, May 1, 2017 at 12:16 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Sun, 30 Apr 2017 23:55:02 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> On Thu, Apr 27, 2017 at 3:51 PM, Nicolas George <george@nsup.org> wrote: >> > L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit : >> >> I agree >> >> in fact i added such a flag in 2011 (4d34b6c1a1254850e39a36f08f4d2730092a54db) >> >> within the API of that time to avfilter >> > >> > It was not a bad idea, but it should not be limited to filters. A few >> > comments. >> > >> > * First, the framequeue framework does not produce unaligned code. >> > According to the C standard, the data it handles stay aligned. The >> > alignment problems come from non-standard requirements by special >> > processor features used by some filters and codecs, but not all. >> > >> > * That means a lot of the most useful codecs and filters will suffer >> > from it, but not all. For many tasks, the alignment is just fine, and >> > the extra copy would be wasteful. >> > >> > * The alignment requirements increase. Before AVX, it was up to 16, now >> > it can be 32, and I have no doubt future processor will at some point >> > require 64 or 128. But realigning buffers used with SSE to 32 would be >> > wasteful too. Thus, we do not require a flag but a full integer. >> > >> > * The code that does the actual work of realigning a buffer should >> > available as a stand-alone API, to be used by applications that work at >> > low-level. I suppose something like that would be in order: >> > >> > int av_frame_realign(AVFrame *frame, unsigned align); >> > >> > Or maybe: >> > >> > int av_frame_realign(AVFrame *frame, unsigned align, >> > AVBufferAllocator *alloc); >> > >> > where AVBufferAllocator is there to allocate possibly hardware or mmaped >> > buffers. >> > >> > * It is another argument for my leitmotiv that filters and codecs are >> > actually the same and should be merged API-wise. >> > >> > * It would be better to have the API just work for everything rather >> > than documenting the alignment needs. >> > >> > As for the actual implementation, I see a lot of different approaches: >> > >> > - have the framework realing the frame before submitting it to the >> > filters and codecs: costly in malloc() and memcpy() but simple; >> > >> > - have each filter or codec call av_frame_realign() as needed; it may >> > seem less elegant than the previous proposal, but it may prove a >> > better choice in the light of what follows; >> > >> > - have each filter or codec copy the unaligned data into a buffer >> > allocated once and for all or on the stack, possibly by small chunks: >> > less costly in malloc() and refcounting overhead, and possibly better >> > cache-locality, but more complex code; >> > >> > - run the plain C version of the code on unaligned data rather than the >> > vectorized version, or the less-vectorized version (SSE vs AVX) on >> > insufficiently aligned data. >> > >> > Since all this boils down to a matter of performance and is related to >> > the core task of FFmpeg, I think the choice between the various options >> > should be done on a case-by-case basis using real benchmarks. >> >> Are you working on these? Because currently I'm not. > > I think it's probably ok to push your current patch, since the original > author of the patch who broke this probably won't fix it. As you said before, my patch didn't solve the problem. Here segfault with volume filter: ./ffmpeg -filter_complex "aevalsrc=0:d=10, aformat=fltp, firequalizer=fixed=on:delay=0.0013, volume=0.5" -f null - Thank's
On Mon, May 1, 2017 at 12:06 AM, Nicolas George <george@nsup.org> wrote: > Le primidi 11 floréal, an CCXXV, Muhammad Faiz a écrit : >> Are you working on these? Because currently I'm not. > > There is nothing to work on yet: the message you answer to is raising a > question about the global design of the internal API. That question > needs an answer before any work can be done, and I can not decide alone. Probably, better to post a patch, choosing one of the designs, and then it will be reviewed rather than waiting an answer here. Thank's.
On 4/30/17, Nicolas George <george@nsup.org> wrote: > Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >> Are you working on these? Because currently I'm not. > > There is nothing to work on yet: the message you answer to is raising a > question about the global design of the internal API. That question > needs an answer before any work can be done, and I can not decide alone. > How nice, introducing bug that causes crash and then claiming there is not such bug. Which filters you consider deemed worthy to not crash?
Hi, On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: > On 4/30/17, Nicolas George <george@nsup.org> wrote: >> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>> Are you working on these? Because currently I'm not. >> >> There is nothing to work on yet: the message you answer to is raising a >> question about the global design of the internal API. That question >> needs an answer before any work can be done, and I can not decide alone. >> > > How nice, introducing bug that causes crash and then claiming there is > not such bug. > > Which filters you consider deemed worthy to not crash? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted until API stuff is sorted. This should also be backported to 3.3 because these issues are present in that release. Kyle
Hi, On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote: > I think it's probably ok to push your current patch, since the original > author of the patch who broke this probably won't fix it. I don't like the patch because it essentially duplicates the C implementation of the DSP function. If we do that here, are we going to do that everywhere? Will we get 2 DSP functions, aligned and unaligned? I foresee a mess. Michael already said that too, I believe. Ronald
Hi, On Mon, May 1, 2017 at 12:44 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote: > >> I think it's probably ok to push your current patch, since the original >> author of the patch who broke this probably won't fix it. > > > I don't like the patch because it essentially duplicates the C > implementation of the DSP function. If we do that here, are we going to do > that everywhere? Will we get 2 DSP functions, aligned and unaligned? I > foresee a mess. > > Michael already said that too, I believe. > Let me expand a little bit further: - if we agree that the API does not require alignment, then we should fix the DSP function (and others in other filters) to allow non-aligned or differently-aligned input data. - if not, then the data alignment should be fixed. Ronald
On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: > Hi, > > On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>> Are you working on these? Because currently I'm not. >>> >>> There is nothing to work on yet: the message you answer to is raising a >>> question about the global design of the internal API. That question >>> needs an answer before any work can be done, and I can not decide alone. >>> >> >> How nice, introducing bug that causes crash and then claiming there is >> not such bug. >> >> Which filters you consider deemed worthy to not crash? >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted > until API stuff is sorted. This should also be backported to 3.3 > because these issues are present in that release. > > Kyle Of course no. Reverting it will make more bug. Unless https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html and https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html are also applied. Thank's
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: >> Hi, >> >> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >>> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>>> Are you working on these? Because currently I'm not. >>>> >>>> There is nothing to work on yet: the message you answer to is raising a >>>> question about the global design of the internal API. That question >>>> needs an answer before any work can be done, and I can not decide alone. >>>> >>> >>> How nice, introducing bug that causes crash and then claiming there is >>> not such bug. >>> >>> Which filters you consider deemed worthy to not crash? >>> _______________________________________________ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted >> until API stuff is sorted. This should also be backported to 3.3 >> because these issues are present in that release. >> >> Kyle > > Of course no. Reverting it will make more bug. > > Unless > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html > and > https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html > are also applied. One of those are already reviewed, other looks like is not needed at all. Could you elaborate why it is needed?
On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote: > On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: >>> Hi, >>> >>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >>>> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>>>> Are you working on these? Because currently I'm not. >>>>> >>>>> There is nothing to work on yet: the message you answer to is raising a >>>>> question about the global design of the internal API. That question >>>>> needs an answer before any work can be done, and I can not decide alone. >>>>> >>>> >>>> How nice, introducing bug that causes crash and then claiming there is >>>> not such bug. >>>> >>>> Which filters you consider deemed worthy to not crash? >>>> _______________________________________________ >>>> ffmpeg-devel mailing list >>>> ffmpeg-devel@ffmpeg.org >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> >>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted >>> until API stuff is sorted. This should also be backported to 3.3 >>> because these issues are present in that release. >>> >>> Kyle >> >> Of course no. Reverting it will make more bug. >> >> Unless >> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html >> and >> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html >> are also applied. > > One of those are already reviewed, other looks like is not needed at all. > Could you elaborate why it is needed? The code before the patch write to unwritable frame. test-case: ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; [a1][b1] vstack' the data will be corrupted because fixed=on enables partial_buf_size stuff Compare that without fixed=on. Thank's
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote: >> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: >>>> Hi, >>>> >>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>>>>> Are you working on these? Because currently I'm not. >>>>>> >>>>>> There is nothing to work on yet: the message you answer to is raising >>>>>> a >>>>>> question about the global design of the internal API. That question >>>>>> needs an answer before any work can be done, and I can not decide >>>>>> alone. >>>>>> >>>>> >>>>> How nice, introducing bug that causes crash and then claiming there is >>>>> not such bug. >>>>> >>>>> Which filters you consider deemed worthy to not crash? >>>>> _______________________________________________ >>>>> ffmpeg-devel mailing list >>>>> ffmpeg-devel@ffmpeg.org >>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> >>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted >>>> until API stuff is sorted. This should also be backported to 3.3 >>>> because these issues are present in that release. >>>> >>>> Kyle >>> >>> Of course no. Reverting it will make more bug. >>> >>> Unless >>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html >>> and >>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html >>> are also applied. >> >> One of those are already reviewed, other looks like is not needed at all. >> Could you elaborate why it is needed? > > The code before the patch write to unwritable frame. > > test-case: > ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, > asplit [a][b]; > [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > [a1][b1] vstack' > > the data will be corrupted because fixed=on enables partial_buf_size stuff > > Compare that without fixed=on. Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2] one, time without providing alternative solution.
On Tue, May 2, 2017 at 1:05 AM, Paul B Mahol <onemda@gmail.com> wrote: > On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >> On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote: >>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >>>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: >>>>> Hi, >>>>> >>>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >>>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>>>>>> Are you working on these? Because currently I'm not. >>>>>>> >>>>>>> There is nothing to work on yet: the message you answer to is raising >>>>>>> a >>>>>>> question about the global design of the internal API. That question >>>>>>> needs an answer before any work can be done, and I can not decide >>>>>>> alone. >>>>>>> >>>>>> >>>>>> How nice, introducing bug that causes crash and then claiming there is >>>>>> not such bug. >>>>>> >>>>>> Which filters you consider deemed worthy to not crash? >>>>>> _______________________________________________ >>>>>> ffmpeg-devel mailing list >>>>>> ffmpeg-devel@ffmpeg.org >>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>> >>>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted >>>>> until API stuff is sorted. This should also be backported to 3.3 >>>>> because these issues are present in that release. >>>>> >>>>> Kyle >>>> >>>> Of course no. Reverting it will make more bug. >>>> >>>> Unless >>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html >>>> and >>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html >>>> are also applied. >>> >>> One of those are already reviewed, other looks like is not needed at all. >>> Could you elaborate why it is needed? >> >> The code before the patch write to unwritable frame. >> >> test-case: >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, >> asplit [a][b]; >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >> [a1][b1] vstack' >> >> the data will be corrupted because fixed=on enables partial_buf_size stuff >> >> Compare that without fixed=on. > > Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2] one, > time without providing alternative solution. The alternative was 383057f8e744efeaaa3648a59bc577b25b055835, of course. I approved it at that time.
On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Tue, May 2, 2017 at 1:05 AM, Paul B Mahol <onemda@gmail.com> wrote: >> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >>> On Tue, May 2, 2017 at 12:45 AM, Paul B Mahol <onemda@gmail.com> wrote: >>>> On 5/1/17, Muhammad Faiz <mfcc64@gmail.com> wrote: >>>>> On Mon, May 1, 2017 at 11:22 PM, Kyle Swanson <k@ylo.ph> wrote: >>>>>> Hi, >>>>>> >>>>>> On Mon, May 1, 2017 at 3:18 AM, Paul B Mahol <onemda@gmail.com> wrote: >>>>>>> On 4/30/17, Nicolas George <george@nsup.org> wrote: >>>>>>>> Le primidi 11 floreal, an CCXXV, Muhammad Faiz a ecrit : >>>>>>>>> Are you working on these? Because currently I'm not. >>>>>>>> >>>>>>>> There is nothing to work on yet: the message you answer to is >>>>>>>> raising >>>>>>>> a >>>>>>>> question about the global design of the internal API. That question >>>>>>>> needs an answer before any work can be done, and I can not decide >>>>>>>> alone. >>>>>>>> >>>>>>> >>>>>>> How nice, introducing bug that causes crash and then claiming there >>>>>>> is >>>>>>> not such bug. >>>>>>> >>>>>>> Which filters you consider deemed worthy to not crash? >>>>>>> _______________________________________________ >>>>>>> ffmpeg-devel mailing list >>>>>>> ffmpeg-devel@ffmpeg.org >>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>>>> >>>>>> Maybe 383057f8e744efeaaa3648a59bc577b25b055835 should be reverted >>>>>> until API stuff is sorted. This should also be backported to 3.3 >>>>>> because these issues are present in that release. >>>>>> >>>>>> Kyle >>>>> >>>>> Of course no. Reverting it will make more bug. >>>>> >>>>> Unless >>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206285.html >>>>> and >>>>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206284.html >>>>> are also applied. >>>> >>>> One of those are already reviewed, other looks like is not needed at >>>> all. >>>> Could you elaborate why it is needed? >>> >>> The code before the patch write to unwritable frame. >>> >>> test-case: >>> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, >>> asplit [a][b]; >>> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >>> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >>> [a1][b1] vstack' >>> >>> the data will be corrupted because fixed=on enables partial_buf_size >>> stuff >>> >>> Compare that without fixed=on. >> >> Hmm, so it seems Nicolas blocked [1/2] patch which is mandatory for [2/2] >> one, >> time without providing alternative solution. > > The alternative was 383057f8e744efeaaa3648a59bc577b25b055835, of > course. I approved it at that time. This is all one big mess.
On Mon, May 1, 2017 at 11:45 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Mon, May 1, 2017 at 12:44 PM, Ronald S. Bultje <rsbultje@gmail.com> > wrote: > >> Hi, >> >> On Sun, Apr 30, 2017 at 1:16 PM, wm4 <nfxjfg@googlemail.com> wrote: >> >>> I think it's probably ok to push your current patch, since the original >>> author of the patch who broke this probably won't fix it. >> >> >> I don't like the patch because it essentially duplicates the C >> implementation of the DSP function. If we do that here, are we going to do >> that everywhere? Will we get 2 DSP functions, aligned and unaligned? I >> foresee a mess. >> >> Michael already said that too, I believe. >> > > Let me expand a little bit further: > - if we agree that the API does not require alignment, then we should fix > the DSP function (and others in other filters) to allow non-aligned or > differently-aligned input data. > - if not, then the data alignment should be fixed. My opinion: - for short term, the data alignment should be fixed. - for long term, the framework should support non-aligned data. No needs for the DSP function to support non-aligned data. Instead ensure the framework to send aligned data to filters/codecs which require it. As Michael suggested, this can be achieved by using flags/required_alignment similar to need_writable. Thank's
Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit :
> This is all one big mess.
It is, but I will not take responsibility when it is not mine.
Libavfilter was in need of a serious overhaul. I do not see you denying
it, and you suffered from the limitations it was causing as much as
anybody else. I have undertaken that overhaul, and the help I received
from other developers in that project has been scarce, to say the least.
I do not complain, I can manage on my own; it will take longer, but I am
in no rush. But I will not let the complaints of the inspectors of
finished work trouble me.
An overhaul of this magnitude is sure to introduce bugs. This one did,
and I made a point of fixing them. My work tree is currently set up to
investigate the problem with shortest and dualinput. Once again, feel
free to offer help.
An overhaul of this magnitude is also sure to reveal bugs that were
already present in the code base and just did not manifest due to
undocumented specifics of the old implementation.
The crash we are discussing is caused by the new code violating the
unwritten un-agreed-upon rule that it must keep the data extra-aligned.
Except there is no such thing as an "unwritten un-agreed-upon rule". The
cause of the crash is the code in libmp3lame relying on this unwritten
un-agreed-upon rule. I strongly suspect it can be triggered by an
application without using libavfilter at all.
For now, fixing libmp3lame is the correct solution, and this patch does
it. I think it is not the best long-term solution, but that is not for
me to say.
Another solution would be to write the rule. If somebody does, then I
will implement the code needed to respect it in libavfilter.
But I will not raise another finger to write the rule, because whatever
I do I will be blamed, and I am fed up with that attitude.
Regards,
On Tue, 2 May 2017 16:16:35 +0200 Nicolas George <george@nsup.org> wrote: > Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit : > > This is all one big mess. > > It is, but I will not take responsibility when it is not mine. > > Libavfilter was in need of a serious overhaul. I do not see you denying > it, and you suffered from the limitations it was causing as much as > anybody else. I have undertaken that overhaul, and the help I received > from other developers in that project has been scarce, to say the least. That doesn't free you from the responsibility to keep things in a reasonably working state. > I do not complain, I can manage on my own; it will take longer, but I am > in no rush. But I will not let the complaints of the inspectors of > finished work trouble me. I can understand that attitude, but if a fix takes "longer" and meanwhile certain filters or encoders crash left and right in git master and even in the latest FFmpeg release, there's a need to act quickly, even if it means reverting certain patches. I don't want to blame anyone (and I ask you not to blame others either, like you did above), but please fix/avoid crashing bugs?
Hi, On Tue, May 2, 2017 at 12:18 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 2 May 2017 16:16:35 +0200 > Nicolas George <george@nsup.org> wrote: > >> Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit : >> > This is all one big mess. >> >> It is, but I will not take responsibility when it is not mine. >> >> Libavfilter was in need of a serious overhaul. I do not see you denying >> it, and you suffered from the limitations it was causing as much as >> anybody else. I have undertaken that overhaul, and the help I received >> from other developers in that project has been scarce, to say the least. > > That doesn't free you from the responsibility to keep things in a > reasonably working state. > >> I do not complain, I can manage on my own; it will take longer, but I am >> in no rush. But I will not let the complaints of the inspectors of >> finished work trouble me. > > I can understand that attitude, but if a fix takes "longer" and > meanwhile certain filters or encoders crash left and right in git > master and even in the latest FFmpeg release, there's a need to act > quickly, even if it means reverting certain patches. I believe this is still broken on git master and is present on release 3.3. If a proper fix is going to take time, the patches that caused this should probably just be reverted for the time being, IMHO. > I don't want to blame anyone (and I ask you not to blame others either, > like you did above), but please fix/avoid crashing bugs? > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Thanks, Kyle
Le quintidi 15 floréal, an CCXXV, Kyle Swanson a écrit : > I believe this is still broken on git master and is present on release Well, nobody fixed it. > 3.3. If a proper fix is going to take time, There is no reason it should, the issue is rather simple. > the patches that caused > this should probably just be reverted for the time being, IMHO. Absolutely not, these change were there for a reason, that reason is still valid. Regards,
On Thu, 4 May 2017 22:24:15 +0200 Nicolas George <george@nsup.org> wrote: > Le quintidi 15 floréal, an CCXXV, Kyle Swanson a écrit : > > I believe this is still broken on git master and is present on release > > Well, nobody fixed it. > > > 3.3. If a proper fix is going to take time, > > There is no reason it should, the issue is rather simple. > > > the patches that caused > > this should probably just be reverted for the time being, IMHO. > > Absolutely not, these change were there for a reason, that reason is > still valid. So I guess our users are supposed to tolerate those crashes? Revert seems like the right thing to do. Your changes can be reapplied once the other issues are fixed. Or you come up with a fix now.
Le quintidi 15 floréal, an CCXXV, Nicolas George a écrit : > Absolutely not, these change were there for a reason, that reason is > still valid. Also, the bug is not in libavfilter, so reverting changes in libavfilter makes no sense. Regards,
On Fri, 5 May 2017 00:05:35 +0200 Nicolas George <george@nsup.org> wrote: > Le quintidi 15 floréal, an CCXXV, Nicolas George a écrit : > > Absolutely not, these change were there for a reason, that reason is > > still valid. > > Also, the bug is not in libavfilter, so reverting changes in libavfilter > makes no sense. Yes, it does, because it triggers the crash. It amazes me that you don't think crashing bugs should be fixed quickly. Nobody cares if your code is awesome and perfect if it trigger crashes.
On Fri, May 5, 2017 at 3:15 AM, Kyle Swanson <k@ylo.ph> wrote: > Hi, > > On Tue, May 2, 2017 at 12:18 PM, wm4 <nfxjfg@googlemail.com> wrote: >> On Tue, 2 May 2017 16:16:35 +0200 >> Nicolas George <george@nsup.org> wrote: >> >>> Le duodi 12 floréal, an CCXXV, Paul B Mahol a écrit : >>> > This is all one big mess. >>> >>> It is, but I will not take responsibility when it is not mine. >>> >>> Libavfilter was in need of a serious overhaul. I do not see you denying >>> it, and you suffered from the limitations it was causing as much as >>> anybody else. I have undertaken that overhaul, and the help I received >>> from other developers in that project has been scarce, to say the least. >> >> That doesn't free you from the responsibility to keep things in a >> reasonably working state. >> >>> I do not complain, I can manage on my own; it will take longer, but I am >>> in no rush. But I will not let the complaints of the inspectors of >>> finished work trouble me. >> >> I can understand that attitude, but if a fix takes "longer" and >> meanwhile certain filters or encoders crash left and right in git >> master and even in the latest FFmpeg release, there's a need to act >> quickly, even if it means reverting certain patches. > > I believe this is still broken on git master and is present on release > 3.3. If a proper fix is going to take time, the patches that caused > this should probably just be reverted for the time being, IMHO. > >> I don't want to blame anyone (and I ask you not to blame others either, >> like you did above), but please fix/avoid crashing bugs? >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > Thanks, > Kyle A new patch is posted. Thank's
diff --git a/libavcodec/libmp3lame.c b/libavcodec/libmp3lame.c index 5e26743..cd505bb 100644 --- a/libavcodec/libmp3lame.c +++ b/libavcodec/libmp3lame.c @@ -203,15 +203,20 @@ static int mp3lame_encode_frame(AVCodecContext *avctx, AVPacket *avpkt, ENCODE_BUFFER(lame_encode_buffer_int, int32_t, frame->data); break; case AV_SAMPLE_FMT_FLTP: - if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8)) { - av_log(avctx, AV_LOG_ERROR, "inadequate AVFrame plane padding\n"); - return AVERROR(EINVAL); - } for (ch = 0; ch < avctx->channels; ch++) { + if (frame->linesize[0] < 4 * FFALIGN(frame->nb_samples, 8) || 0x1F & (intptr_t)(frame->data[ch])) { + float *src = (float *) frame->data[ch]; + float *dst = s->samples_flt[ch]; + int k; + + for (k = 0; k < frame->nb_samples; k++) + dst[k] = src[k] * 32768.0f; + } else { s->fdsp->vector_fmul_scalar(s->samples_flt[ch], (const float *)frame->data[ch], 32768.0f, FFALIGN(frame->nb_samples, 8)); + } } ENCODE_BUFFER(lame_encode_buffer_float, float, s->samples_flt); break;
This should fix Ticket6349 Since 383057f8e744efeaaa3648a59bc577b25b055835, framequeue may generate unaligned frame data. Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> --- libavcodec/libmp3lame.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)