| Message ID | 20170130003702.26686-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | d25769555bb3e73c811da05d309856883ff41a9f |
| Headers | show |
Le primidi 11 pluviôse, an CCXXV, Marton Balint a écrit : > I wonder how unknown layouts ever worked without this? > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/frame.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) LGTM, but I do not maintain that file. Regards,
On Mon, Jan 30, 2017 at 1:37 AM, Marton Balint <cus@passwd.hu> wrote: > I wonder how unknown layouts ever worked without this? > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/frame.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index c2f5509..a08e0c5 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) > > if (dst->width > 0 && dst->height > 0) > return frame_copy_video(dst, src); > - else if (dst->nb_samples > 0 && dst->channel_layout) > + else if (dst->nb_samples > 0 && dst->channels > 0) > return frame_copy_audio(dst, src); > > return AVERROR(EINVAL); > -- > 2.10.2 Looks good, the copy routine uses dst->channels for copying, not channel_layout, so that should also be whats checked. - Hendrik
On Mon, 30 Jan 2017 01:37:02 +0100 Marton Balint <cus@passwd.hu> wrote: > I wonder how unknown layouts ever worked without this? > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/frame.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index c2f5509..a08e0c5 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) > > if (dst->width > 0 && dst->height > 0) > return frame_copy_video(dst, src); > - else if (dst->nb_samples > 0 && dst->channel_layout) > + else if (dst->nb_samples > 0 && dst->channels > 0) > return frame_copy_audio(dst, src); > > return AVERROR(EINVAL); The original code was written with the assumption that only channel layouts exist in the AVFrame. And the Libav API follows that. This patch will probably break Libav API users (or those who want to stay compatible to them) some more.
On Mon, Jan 30, 2017 at 8:40 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 30 Jan 2017 01:37:02 +0100 > Marton Balint <cus@passwd.hu> wrote: > >> I wonder how unknown layouts ever worked without this? >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavutil/frame.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >> index c2f5509..a08e0c5 100644 >> --- a/libavutil/frame.c >> +++ b/libavutil/frame.c >> @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) >> >> if (dst->width > 0 && dst->height > 0) >> return frame_copy_video(dst, src); >> - else if (dst->nb_samples > 0 && dst->channel_layout) >> + else if (dst->nb_samples > 0 && dst->channels > 0) >> return frame_copy_audio(dst, src); >> >> return AVERROR(EINVAL); > > The original code was written with the assumption that only channel > layouts exist in the AVFrame. And the Libav API follows that. This > patch will probably break Libav API users (or those who want to stay > compatible to them) some more. The copying code would have failed anyway if channels was 0 and not copied anything, so it doesn't break anything that worked before - except that it returns an error code now properly indicating the failure. In any case, we've decide quite a while ago to forego any illusions of API or ABI compatibility, because its impossible to guarantee it anyway. - Hendrik
On Mon, 30 Jan 2017 08:47:49 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Mon, Jan 30, 2017 at 8:40 AM, wm4 <nfxjfg@googlemail.com> wrote: > > On Mon, 30 Jan 2017 01:37:02 +0100 > > Marton Balint <cus@passwd.hu> wrote: > > > >> I wonder how unknown layouts ever worked without this? > >> > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> libavutil/frame.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/libavutil/frame.c b/libavutil/frame.c > >> index c2f5509..a08e0c5 100644 > >> --- a/libavutil/frame.c > >> +++ b/libavutil/frame.c > >> @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) > >> > >> if (dst->width > 0 && dst->height > 0) > >> return frame_copy_video(dst, src); > >> - else if (dst->nb_samples > 0 && dst->channel_layout) > >> + else if (dst->nb_samples > 0 && dst->channels > 0) > >> return frame_copy_audio(dst, src); > >> > >> return AVERROR(EINVAL); > > > > The original code was written with the assumption that only channel > > layouts exist in the AVFrame. And the Libav API follows that. This > > patch will probably break Libav API users (or those who want to stay > > compatible to them) some more. > > The copying code would have failed anyway if channels was 0 and not > copied anything, so it doesn't break anything that worked before - > except that it returns an error code now properly indicating the > failure. Hm, ok then. > In any case, we've decide quite a while ago to forego any illusions of > API or ABI compatibility, because its impossible to guarantee it > anyway. We still have API compatibility. The channels issue is one of the only things that work radically different from Libav. But it looks like this was broken long ago and we probably don't need to bother to fix it.
On 1/30/17, wm4 <nfxjfg@googlemail.com> wrote: > On Mon, 30 Jan 2017 08:47:49 +0100 > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Mon, Jan 30, 2017 at 8:40 AM, wm4 <nfxjfg@googlemail.com> wrote: >> > On Mon, 30 Jan 2017 01:37:02 +0100 >> > Marton Balint <cus@passwd.hu> wrote: >> > >> >> I wonder how unknown layouts ever worked without this? >> >> >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> >> --- >> >> libavutil/frame.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >> >> index c2f5509..a08e0c5 100644 >> >> --- a/libavutil/frame.c >> >> +++ b/libavutil/frame.c >> >> @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) >> >> >> >> if (dst->width > 0 && dst->height > 0) >> >> return frame_copy_video(dst, src); >> >> - else if (dst->nb_samples > 0 && dst->channel_layout) >> >> + else if (dst->nb_samples > 0 && dst->channels > 0) >> >> return frame_copy_audio(dst, src); >> >> >> >> return AVERROR(EINVAL); >> > >> > The original code was written with the assumption that only channel >> > layouts exist in the AVFrame. And the Libav API follows that. This >> > patch will probably break Libav API users (or those who want to stay >> > compatible to them) some more. >> >> The copying code would have failed anyway if channels was 0 and not >> copied anything, so it doesn't break anything that worked before - >> except that it returns an error code now properly indicating the >> failure. > > Hm, ok then. > >> In any case, we've decide quite a while ago to forego any illusions of >> API or ABI compatibility, because its impossible to guarantee it >> anyway. > > We still have API compatibility. The channels issue is one of the > only things that work radically different from Libav. But it looks like > this was broken long ago and we probably don't need to bother to fix it. The issue should be fixed, patch LGTM.
On Mon, 30 Jan 2017, Paul B Mahol wrote: > On 1/30/17, wm4 <nfxjfg@googlemail.com> wrote: >> On Mon, 30 Jan 2017 08:47:49 +0100 >> Hendrik Leppkes <h.leppkes@gmail.com> wrote: >> >>> On Mon, Jan 30, 2017 at 8:40 AM, wm4 <nfxjfg@googlemail.com> wrote: >>> > On Mon, 30 Jan 2017 01:37:02 +0100 >>> > Marton Balint <cus@passwd.hu> wrote: >>> > >>> >> I wonder how unknown layouts ever worked without this? >>> >> >>> >> Signed-off-by: Marton Balint <cus@passwd.hu> >>> >> --- >>> >> libavutil/frame.c | 2 +- >>> >> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >> >>> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >>> >> index c2f5509..a08e0c5 100644 >>> >> --- a/libavutil/frame.c >>> >> +++ b/libavutil/frame.c >>> >> @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) >>> >> >>> >> if (dst->width > 0 && dst->height > 0) >>> >> return frame_copy_video(dst, src); >>> >> - else if (dst->nb_samples > 0 && dst->channel_layout) >>> >> + else if (dst->nb_samples > 0 && dst->channels > 0) >>> >> return frame_copy_audio(dst, src); >>> >> >>> >> return AVERROR(EINVAL); >>> > >>> > The original code was written with the assumption that only channel >>> > layouts exist in the AVFrame. And the Libav API follows that. This >>> > patch will probably break Libav API users (or those who want to stay >>> > compatible to them) some more. >>> >>> The copying code would have failed anyway if channels was 0 and not >>> copied anything, so it doesn't break anything that worked before - >>> except that it returns an error code now properly indicating the >>> failure. >> >> Hm, ok then. >> >>> In any case, we've decide quite a while ago to forego any illusions of >>> API or ABI compatibility, because its impossible to guarantee it >>> anyway. >> >> We still have API compatibility. The channels issue is one of the >> only things that work radically different from Libav. But it looks like >> this was broken long ago and we probably don't need to bother to fix it. > > The issue should be fixed, patch LGTM. Thanks everyone, pushed. Regards, Marton
diff --git a/libavutil/frame.c b/libavutil/frame.c index c2f5509..a08e0c5 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -725,7 +725,7 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src) if (dst->width > 0 && dst->height > 0) return frame_copy_video(dst, src); - else if (dst->nb_samples > 0 && dst->channel_layout) + else if (dst->nb_samples > 0 && dst->channels > 0) return frame_copy_audio(dst, src); return AVERROR(EINVAL);
I wonder how unknown layouts ever worked without this? Signed-off-by: Marton Balint <cus@passwd.hu> --- libavutil/frame.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)