Re: [FFmpeg-devel] [PATCH] avcodec/libmp3lame: properly handle unaligned frame data
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, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/2] avcodec/svq3: Increase offsets to prevent integer overflows
Fixes: 1280/clusterfuzz-testcase-minimized-6102353767825408
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer
---
libavcodec/svq3.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
index 06e3d37590..3e35fd73d6 100644
--- a/libavcodec/svq3.c
+++ b/libavcodec/svq3.c
@@ -562,8 +562,8 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size, int
mode,
int fx, fy;
mx = (mx + 1 >> 1) + dx;
my = (my + 1 >> 1) + dy;
-fx = (unsigned)(mx + 0x3000) / 3 - 0x1000;
-fy = (unsigned)(my + 0x3000) / 3 - 0x1000;
+fx = (unsigned)(mx + 0x3) / 3 - 0x1;
+fy = (unsigned)(my + 0x3) / 3 - 0x1;
dxy = (mx - 3 * fx) + 4 * (my - 3 * fy);
svq3_mc_dir_part(s, x, y, part_width, part_height,
@@ -571,8 +571,8 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size, int
mode,
mx += mx;
my += my;
} else if (mode == HALFPEL_MODE || mode == PREDICT_MODE) {
-mx = (unsigned)(mx + 1 + 0x3000) / 3 + dx - 0x1000;
-my = (unsigned)(my + 1 + 0x3000) / 3 + dy - 0x1000;
+mx = (unsigned)(mx + 1 + 0x3) / 3 + dx - 0x1;
+my = (unsigned)(my + 1 + 0x3) / 3 + dy - 0x1;
dxy = (mx & 1) + 2 * (my & 1);
svq3_mc_dir_part(s, x, y, part_width, part_height,
@@ -580,8 +580,8 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size, int
mode,
mx *= 3;
my *= 3;
} else {
-mx = (unsigned)(mx + 3 + 0x6000) / 6 + dx - 0x1000;
-my = (unsigned)(my + 3 + 0x6000) / 6 + dy - 0x1000;
+mx = (unsigned)(mx + 3 + 0x6) / 6 + dx - 0x1;
+my = (unsigned)(my + 3 + 0x6) / 6 + dy - 0x1;
svq3_mc_dir_part(s, x, y, part_width, part_height,
mx, my, 0, 0, dir, avg);
--
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
The code does use 16bit sized arrays later so larger deltas would not work
Signed-off-by: Michael Niedermayer
---
libavcodec/svq3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
index 3e35fd73d6..76a465b9c0 100644
--- a/libavcodec/svq3.c
+++ b/libavcodec/svq3.c
@@ -551,7 +551,7 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size, int
mode,
dy = get_interleaved_se_golomb(&s->gb_slice);
dx = get_interleaved_se_golomb(&s->gb_slice);
-if (dx == INVALID_VLC || dy == INVALID_VLC) {
+if (dx != (int16_t)dx || dy != (int16_t)dy) {
av_log(s->avctx, AV_LOG_ERROR, "invalid MV vlc\n");
return -1;
}
--
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit :
> The code does use 16bit sized arrays later so larger deltas would not work
>
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/svq3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
> index 3e35fd73d6..76a465b9c0 100644
> --- a/libavcodec/svq3.c
> +++ b/libavcodec/svq3.c
> @@ -551,7 +551,7 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size,
> int mode,
> dy = get_interleaved_se_golomb(&s->gb_slice);
> dx = get_interleaved_se_golomb(&s->gb_slice);
>
> -if (dx == INVALID_VLC || dy == INVALID_VLC) {
> +if (dx != (int16_t)dx || dy != (int16_t)dy) {
Is this cast not an undefined behaviour if dx is not in the range?
An explicit "& 0x7FFF" may be better. Or using uint16_t, I do not know
the code.
> av_log(s->avctx, AV_LOG_ERROR, "invalid MV vlc\n");
> return -1;
> }
Regards,
--
Nicolas George
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
On Thu, Apr 27, 2017 at 03:22:53PM +0200, Nicolas George wrote:
> L'octidi 8 floréal, an CCXXV, Michael Niedermayer a écrit :
> > The code does use 16bit sized arrays later so larger deltas would not work
> >
> > Signed-off-by: Michael Niedermayer
> > ---
> > libavcodec/svq3.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
> > index 3e35fd73d6..76a465b9c0 100644
> > --- a/libavcodec/svq3.c
> > +++ b/libavcodec/svq3.c
> > @@ -551,7 +551,7 @@ static inline int svq3_mc_dir(SVQ3Context *s, int size,
> > int mode,
> > dy = get_interleaved_se_golomb(&s->gb_slice);
> > dx = get_interleaved_se_golomb(&s->gb_slice);
> >
> > -if (dx == INVALID_VLC || dy == INVALID_VLC) {
>
> > +if (dx != (int16_t)dx || dy != (int16_t)dy) {
>
> Is this cast not an undefined behaviour if dx is not in the range?
It is implementation defined (6.3.1.3.3 ISO/IEC 9899:TC3)
We require Twos-complement style definition for signed integers
>
> An explicit "& 0x7FFF" may be better. Or using uint16_t, I do not know
> the code.
The value is signed, so these will not work without additional code
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/movenc: Explicitly address potential division by zero.
On Fri, Apr 21, 2017 at 12:01:01AM -0700, Lucas Cooper wrote: > Does this need any more work or explanation? The code looks duplicated twice, it should be put in a seperate function, also the indention depth of one of the added lines mismatches the existing code [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB What does censorship reveal? It reveals fear. -- Julian Assange signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
Hi, On Thu, Apr 27, 2017 at 9:56 AM, Michael Niedermayer wrote: > We require Twos-complement style definition for signed integers Do we? Where is that documented? https://www.google.com/search?q=twos-complement+ffmpeg+site:ffmpeg.org+-pipermail Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
L'octidi 8 floréal, an CCXXV, Ronald S. Bultje a écrit : > Do we? Where is that documented? > > https://www.google.com/search?q=twos-complement+ffmpeg+site:ffmpeg.org+-pipermail I think it is not documented anywhere but in the head of various developers, but I remember at least one other instance. And I think it is a very reasonable assumption, at least for certain parts of the code. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/svq3: Reject dx/dy beyond 16bit
Hi, On Thu, Apr 27, 2017 at 10:26 AM, Nicolas George wrote: > L'octidi 8 floréal, an CCXXV, Ronald S. Bultje a écrit : > > Do we? Where is that documented? > > > > https://www.google.com/search?q=twos-complement+ffmpeg+site: > ffmpeg.org+-pipermail > > I think it is not documented anywhere but in the head of various > developers, but I remember at least one other instance. > > And I think it is a very reasonable assumption, at least for certain > parts of the code. I agree, I've heard it before as well. I think it's good to document these things publicly so that there's no risk for partisan bickering. :) Ronald ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/movenc: Explicitly address potential division by zero.
find_fps attempts to infer framerate from AVCodec's timebase. When this
results in a frame rate that isn't explicitly marked as supported in
av_timecode_check_frame_rate, find_fps returns the AVStream's
avg_frame_rate, which, per avformat.h, _may_ be set (or not).
mov_get_mpeg2_xdcam_codec_tag, mov_get_h264_codec_tag and
find_compressor attempt to call av_q2d on the return value of find_fps,
which in the above case, may result in division by zero and therefore,
an undefined frame rate when NaN is converted to int.
---
libavformat/movenc.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index e6e2313c53..a5c6ab88f1 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1321,12 +1321,21 @@ static AVRational find_fps(AVFormatContext *s, AVStream
*st)
return rate;
}
+static int defined_frame_rate(AVFormatContext *s, AVStream *st)
+{
+AVRational rational_framerate = find_fps(s, st);
+int rate = 0;
+if (rational_framerate.den != 0)
+ rate = av_q2d(rational_framerate);
+return rate;
+}
+
static int mov_get_mpeg2_xdcam_codec_tag(AVFormatContext *s, MOVTrack *track)
{
int tag = track->par->codec_tag;
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(s, st));
+int rate = defined_frame_rate(s, st);
if (!tag)
tag = MKTAG('m', '2', 'v', '1'); //fallback tag
@@ -1388,7 +1397,7 @@ static int mov_get_h264_codec_tag(AVFormatContext *s,
MOVTrack *track)
int tag = track->par->codec_tag;
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(s, st));
+int rate = defined_frame_rate(s, st);
if (!tag)
tag = MKTAG('a', 'v', 'c', 'i'); //fallback tag
@@ -1850,7 +1859,7 @@ static void find_compressor(char * compressor_name, int
len, MOVTrack *track)
} else if (track->par->codec_id == AV_CODEC_ID_MPEG2VIDEO && xdcam_res) {
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(NULL, st));
+int rate = defined_frame_rate(NULL, st);
av_strlcatf(compressor_name, len, "XDCAM");
if (track->par->format == AV_PIX_FMT_YUV422P) {
av_strlcatf(compressor_name, len, " HD422");
--
2.13.0.rc0.306.g87b477812d-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] pthread_frame: set err from the thread that return frame
On Thu, Apr 27, 2017 at 2:59 AM, wm4 wrote: > On Thu, 27 Apr 2017 01:20:53 +0700 > Muhammad Faiz wrote: > >> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje >> wrote: >> > Hi, >> > >> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz wrote: >> > >> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 wrote: >> >> > On Tue, 25 Apr 2017 23:52:04 +0700 >> >> > Muhammad Faiz wrote: >> >> > >> >> >> when frame is received, not from other threads. >> >> >> >> >> >> Should fix fate failure with THREADS>=4: >> >> >> make fate-h264-attachment-631 THREADS=4 >> >> >> >> >> >> Signed-off-by: Muhammad Faiz >> >> >> --- >> >> >> libavcodec/pthread_frame.c | 4 >> >> >> 1 file changed, 4 insertions(+) >> >> >> >> >> >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c >> >> >> index 13d6828..c452ed7 100644 >> >> >> --- a/libavcodec/pthread_frame.c >> >> >> +++ b/libavcodec/pthread_frame.c >> >> >> @@ -547,6 +547,10 @@ int ff_thread_decode_frame(AVCodecContext *avctx, >> >> >> >> >> >> fctx->next_finished = finished; >> >> >> >> >> >> +/* if frame is returned, properly set err from the thread that >> >> return frame */ >> >> >> +if (*got_picture_ptr) >> >> >> +err = p->result; >> >> >> + >> >> >> /* return the size of the consumed packet if no error occurred */ >> >> >> if (err >= 0) >> >> >> err = avpkt->size; >> >> > >> >> > Well, the logic confuses me. Does this override an earlier set err >> >> > value? >> >> >> >> Yes, because an earlier set err value may be from a different thread. >> >> >> >> >Could err be set to the correct value in the first place (inside >> >> > of the loop)? >> >> >> >> No, it was intended on 32a5b631267 >> > >> > >> > Thanks for working on this. It's good to get more people familiar with this >> > code. >> > >> > So, I'm looking at understanding this, you're trying to fix the case where >> > during draining, we may iterate over >1 worker thread cases where the first >> > returned an error code without having decoded a frame, and the second >> > decoded a frame without returning an error code, right? The current code >> > would return a frame with an error return code, which I believe is then >> > ignored by the user thread. >> > >> > So, you're basically trying to say that instead, we should ignore the >> > error. I agree that fixes the issue of md5 mismatch w/ vs. w/o threads, but >> > I doubt that it's fundamentally more correct, because the user thread still >> > misses out on error codes from the worker threads. Wouldn't you agree that >> > we should - even during draining - not iterate over N threads, but just the >> > next thread, and return either an error or a decoded frame, and keep doing >> > that until all worker threads are flushed, which we can then signal e.g. by >> > return=0 and *got_picture_ptr=0? >> >> The problem is that return<0 and *got_picture_ptr==0 is also >> considered as eof when avpkt->size==0. > > The old public decode API, or the new API? The latter would be about > the wrapper. I remember adding a hack to avoid the situation that > decoders can get "stuck" during draining, not sure if it was > overwritten in a recent merge. New API, of course. For old api, it depends on who interprets return value and got_frame value. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] pthread_frame: set err from the thread that return frame
On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint wrote: > > > On Wed, 26 Apr 2017, Ronald S. Bultje wrote: > >> Hi, >> >> On Wed, Apr 26, 2017 at 2:20 PM, Muhammad Faiz wrote: >> >>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje >>> wrote: >>> > Hi, >>> > >>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz >>> > wrote: >>> > >>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 wrote: >>> >> > On Tue, 25 Apr 2017 23:52:04 +0700 >>> >> > Muhammad Faiz wrote: >>> >> > >>> >> >> when frame is received, not from other threads. >>> >> >> >>> >> >> Should fix fate failure with THREADS>=4: >>> >> >> make fate-h264-attachment-631 THREADS=4 >>> >> >> >>> >> >> Signed-off-by: Muhammad Faiz >>> >> >> --- >>> >> >> libavcodec/pthread_frame.c | 4 >>> >> >> 1 file changed, 4 insertions(+) >>> >> >> >>> >> >> diff --git a/libavcodec/pthread_frame.c >>> >> >> b/libavcodec/pthread_frame.c >>> >> >> index 13d6828..c452ed7 100644 >>> >> >> --- a/libavcodec/pthread_frame.c >>> >> >> +++ b/libavcodec/pthread_frame.c >>> >> >> @@ -547,6 +547,10 @@ int ff_thread_decode_frame(AVCodecContext >>> *avctx, >>> >> >> >>> >> >> fctx->next_finished = finished; >>> >> >> >>> >> >> +/* if frame is returned, properly set err from the thread that >>> >> return frame */ >>> >> >> +if (*got_picture_ptr) >>> >> >> +err = p->result; >>> >> >> + >>> >> >> /* return the size of the consumed packet if no error occurred >>> */ >>> >> >> if (err >= 0) >>> >> >> err = avpkt->size; >>> >> > >>> >> > Well, the logic confuses me. Does this override an earlier set err >>> >> > value? >>> >> >>> >> Yes, because an earlier set err value may be from a different thread. >>> >> >>> >> >Could err be set to the correct value in the first place (inside >>> >> > of the loop)? >>> >> >>> >> No, it was intended on 32a5b631267 >>> > >>> > >>> > Thanks for working on this. It's good to get more people familiar with >>> this >>> > code. >>> > >>> > So, I'm looking at understanding this, you're trying to fix the case >>> where >>> > during draining, we may iterate over >1 worker thread cases where the >>> first >>> > returned an error code without having decoded a frame, and the second >>> > decoded a frame without returning an error code, right? The current >>> > code >>> > would return a frame with an error return code, which I believe is then >>> > ignored by the user thread. >>> > >>> > So, you're basically trying to say that instead, we should ignore the >>> > error. I agree that fixes the issue of md5 mismatch w/ vs. w/o threads, >>> but >>> > I doubt that it's fundamentally more correct, because the user thread >>> still >>> > misses out on error codes from the worker threads. Wouldn't you agree >>> that >>> > we should - even during draining - not iterate over N threads, but just >>> the >>> > next thread, and return either an error or a decoded frame, and keep >>> doing >>> > that until all worker threads are flushed, which we can then signal >>> > e.g. >>> by >>> > return=0 and *got_picture_ptr=0? >>> >>> The problem is that return<0 and *got_picture_ptr==0 is also >>> considered as eof when avpkt->size==0. >> >> >> >> This will probably count as an API change then, but my thinking is that we >> should add a new API that "fixes" the above. The old API can then skip >> error-packets-on-flush (similar to how your patch does it). >> >> Or do people dislike this? > > > I propose the following: > > Using the old (and deprecated) public API you should simply get an error. > Losing more frames in the end if threading is invovled is acceptable IMHO. > Sliently ignoring an error is not. Error is not silently ignored. Only reordered, and returned after all frames are flushed > > Using the new public API you should get the error code, then the proper > frame on the next call. In the new public API only AVERROR_EOF signals EOF, > so this seems doable. Sound good. Are all decoders ready for this? I mean, a guarantee that they don't return error infinitely on eof? > > Or do I miss something? Or I just stated the obvious? :) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] pthread_frame: set err from the thread that return frame
On Fri, Apr 28, 2017 at 12:16 AM, Muhammad Faiz wrote: > On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint wrote: >> >> >> On Wed, 26 Apr 2017, Ronald S. Bultje wrote: >> >>> Hi, >>> >>> On Wed, Apr 26, 2017 at 2:20 PM, Muhammad Faiz wrote: >>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje wrote: > Hi, > > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz > wrote: > >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 wrote: >> > On Tue, 25 Apr 2017 23:52:04 +0700 >> > Muhammad Faiz wrote: >> > >> >> when frame is received, not from other threads. >> >> >> >> Should fix fate failure with THREADS>=4: >> >> make fate-h264-attachment-631 THREADS=4 >> >> >> >> Signed-off-by: Muhammad Faiz >> >> --- >> >> libavcodec/pthread_frame.c | 4 >> >> 1 file changed, 4 insertions(+) >> >> >> >> diff --git a/libavcodec/pthread_frame.c >> >> b/libavcodec/pthread_frame.c >> >> index 13d6828..c452ed7 100644 >> >> --- a/libavcodec/pthread_frame.c >> >> +++ b/libavcodec/pthread_frame.c >> >> @@ -547,6 +547,10 @@ int ff_thread_decode_frame(AVCodecContext *avctx, >> >> >> >> fctx->next_finished = finished; >> >> >> >> +/* if frame is returned, properly set err from the thread that >> return frame */ >> >> +if (*got_picture_ptr) >> >> +err = p->result; >> >> + >> >> /* return the size of the consumed packet if no error occurred */ >> >> if (err >= 0) >> >> err = avpkt->size; >> > >> > Well, the logic confuses me. Does this override an earlier set err >> > value? >> >> Yes, because an earlier set err value may be from a different thread. >> >> >Could err be set to the correct value in the first place (inside >> > of the loop)? >> >> No, it was intended on 32a5b631267 > > > Thanks for working on this. It's good to get more people familiar with this > code. > > So, I'm looking at understanding this, you're trying to fix the case where > during draining, we may iterate over >1 worker thread cases where the first > returned an error code without having decoded a frame, and the second > decoded a frame without returning an error code, right? The current > code > would return a frame with an error return code, which I believe is then > ignored by the user thread. > > So, you're basically trying to say that instead, we should ignore the > error. I agree that fixes the issue of md5 mismatch w/ vs. w/o threads, but > I doubt that it's fundamentally more correct, because the user thread still > misses out on error codes from the worker threads. Wouldn't you agree that > we should - even during draining - not iterate over N threads, but just the > next thread, and return either an error or a decoded frame, and keep doing > that until all worker threads are flushed, which we can then signal > e.g. by > return=0 and *got_picture_ptr=0? The problem is that return<0 and *got_picture_ptr==0 is also considered as eof when avpkt->size==0. >>> >>> >>> >>> This will probably count as an API change then, but my thinking is that we >>> should add a new API that "fixes" the above. The old API can then skip >>> error-packets-on-flush (similar to how your patch does it). >>> >>> Or do people dislike this? >> >> >> I propose the following: >> >> Using the old (and deprecated) public API you should simply get an error. >> Losing more frames in the end if threading is invovled is acceptable IMHO. >> Sliently ignoring an error is not. > > Error is not silently ignored. Only reordered, and returned after all > frames are flushed > >> >> Using the new public API you should get the error code, then the proper >> frame on the next call. In the new public API only AVERROR_EOF signals EOF, >> so this seems doable. > > Sound good. Are all decoders ready for this? I mean, a guarantee that > they don't return error infinitely on eof? > A Patch is sent. Thx. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/pthread_frame, decode: allow errors to happen on draining
So, all frames and errors are correctly reported in order.
Fix fate failure with THREADS>=4:
make fate-h264-attachment-631 THREADS=4
Suggested-by: wm4, Ronald S. Bultje, Marton Balint
Signed-off-by: Muhammad Faiz
---
libavcodec/decode.c| 3 ++-
libavcodec/pthread_frame.c | 15 +++
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 6ff3c40..f4d32e3 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -568,7 +568,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
avctx->time_base = av_inv_q(av_mul_q(avctx->framerate,
(AVRational){avctx->ticks_per_frame, 1}));
#endif
-if (avctx->internal->draining && !got_frame)
+/* do not stop draining when got_frame != 0 or ret < 0 */
+if (avctx->internal->draining && !got_frame && ret >= 0)
avci->draining_done = 1;
avci->compat_decode_consumed += ret;
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 13d6828..363b139 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -509,8 +509,8 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
/*
* Return the next available frame from the oldest thread.
* If we're at the end of the stream, then we have to skip threads that
- * didn't output a frame, because we don't want to accidentally signal
- * EOF (avpkt->size == 0 && *got_picture_ptr == 0).
+ * didn't output a frame/error, because we don't want to accidentally
signal
+ * EOF (avpkt->size == 0 && *got_picture_ptr == 0 && err >= 0).
*/
do {
@@ -526,20 +526,19 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
av_frame_move_ref(picture, p->frame);
*got_picture_ptr = p->got_frame;
picture->pkt_dts = p->avpkt.dts;
-
-if (p->result < 0)
-err = p->result;
+err = p->result;
/*
* A later call with avkpt->size == 0 may loop over all threads,
- * including this one, searching for a frame to return before being
+ * including this one, searching for a frame/error to return before
being
* stopped by the "finished != fctx->next_finished" condition.
- * Make sure we don't mistakenly return the same frame again.
+ * Make sure we don't mistakenly return the same frame/error again.
*/
p->got_frame = 0;
+p->result = 0;
if (finished >= avctx->thread_count) finished = 0;
-} while (!avpkt->size && !*got_picture_ptr && finished !=
fctx->next_finished);
+} while (!avpkt->size && !*got_picture_ptr && err >= 0 && finished !=
fctx->next_finished);
update_context_from_thread(avctx, p->avctx, 1);
--
2.9.3
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avformat/movenc: Explicitly address potential division by zero.
find_fps attempts to infer framerate from AVCodec's timebase. When this
results in a frame rate that isn't explicitly marked as supported in
av_timecode_check_frame_rate, find_fps returns the AVStream's
avg_frame_rate, which, per avformat.h, _may_ be set (or not).
mov_get_mpeg2_xdcam_codec_tag, mov_get_h264_codec_tag and
find_compressor attempt to call av_q2d on the return value of find_fps,
which in the above case, may result in division by zero and therefore,
an undefined frame rate when NaN is converted to int.
---
libavformat/movenc.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index e6e2313c53..d20d272f89 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1321,12 +1321,21 @@ static AVRational find_fps(AVFormatContext *s, AVStream
*st)
return rate;
}
+static int defined_frame_rate(AVFormatContext *s, AVStream *st)
+{
+AVRational rational_framerate = find_fps(s, st);
+int rate = 0;
+if (rational_framerate.den != 0)
+rate = av_q2d(rational_framerate);
+return rate;
+}
+
static int mov_get_mpeg2_xdcam_codec_tag(AVFormatContext *s, MOVTrack *track)
{
int tag = track->par->codec_tag;
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(s, st));
+int rate = defined_frame_rate(s, st);
if (!tag)
tag = MKTAG('m', '2', 'v', '1'); //fallback tag
@@ -1388,7 +1397,7 @@ static int mov_get_h264_codec_tag(AVFormatContext *s,
MOVTrack *track)
int tag = track->par->codec_tag;
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(s, st));
+int rate = defined_frame_rate(s, st);
if (!tag)
tag = MKTAG('a', 'v', 'c', 'i'); //fallback tag
@@ -1850,7 +1859,7 @@ static void find_compressor(char * compressor_name, int
len, MOVTrack *track)
} else if (track->par->codec_id == AV_CODEC_ID_MPEG2VIDEO && xdcam_res) {
int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
AVStream *st = track->st;
-int rate = av_q2d(find_fps(NULL, st));
+int rate = defined_frame_rate(NULL, st);
av_strlcatf(compressor_name, len, "XDCAM");
if (track->par->format == AV_PIX_FMT_YUV422P) {
av_strlcatf(compressor_name, len, " HD422");
--
2.13.0.rc0.306.g87b477812d-goog
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/movenc: Explicitly address potential division by zero.
I realized that I missed the indentation. I've fixed it in the latest
patch.
On 27 April 2017 at 09:52, Lucas Cooper wrote:
> find_fps attempts to infer framerate from AVCodec's timebase. When this
> results in a frame rate that isn't explicitly marked as supported in
> av_timecode_check_frame_rate, find_fps returns the AVStream's
> avg_frame_rate, which, per avformat.h, _may_ be set (or not).
>
> mov_get_mpeg2_xdcam_codec_tag, mov_get_h264_codec_tag and
> find_compressor attempt to call av_q2d on the return value of find_fps,
> which in the above case, may result in division by zero and therefore,
> an undefined frame rate when NaN is converted to int.
> ---
> libavformat/movenc.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index e6e2313c53..a5c6ab88f1 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -1321,12 +1321,21 @@ static AVRational find_fps(AVFormatContext *s,
> AVStream *st)
> return rate;
> }
>
> +static int defined_frame_rate(AVFormatContext *s, AVStream *st)
> +{
> +AVRational rational_framerate = find_fps(s, st);
> +int rate = 0;
> +if (rational_framerate.den != 0)
> + rate = av_q2d(rational_framerate);
> +return rate;
> +}
> +
> static int mov_get_mpeg2_xdcam_codec_tag(AVFormatContext *s, MOVTrack
> *track)
> {
> int tag = track->par->codec_tag;
> int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
> AVStream *st = track->st;
> -int rate = av_q2d(find_fps(s, st));
> +int rate = defined_frame_rate(s, st);
>
> if (!tag)
> tag = MKTAG('m', '2', 'v', '1'); //fallback tag
> @@ -1388,7 +1397,7 @@ static int mov_get_h264_codec_tag(AVFormatContext
> *s, MOVTrack *track)
> int tag = track->par->codec_tag;
> int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
> AVStream *st = track->st;
> -int rate = av_q2d(find_fps(s, st));
> +int rate = defined_frame_rate(s, st);
>
> if (!tag)
> tag = MKTAG('a', 'v', 'c', 'i'); //fallback tag
> @@ -1850,7 +1859,7 @@ static void find_compressor(char * compressor_name,
> int len, MOVTrack *track)
> } else if (track->par->codec_id == AV_CODEC_ID_MPEG2VIDEO &&
> xdcam_res) {
> int interlaced = track->par->field_order > AV_FIELD_PROGRESSIVE;
> AVStream *st = track->st;
> -int rate = av_q2d(find_fps(NULL, st));
> +int rate = defined_frame_rate(NULL, st);
> av_strlcatf(compressor_name, len, "XDCAM");
> if (track->par->format == AV_PIX_FMT_YUV422P) {
> av_strlcatf(compressor_name, len, " HD422");
> --
> 2.13.0.rc0.306.g87b477812d-goog
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] doc/developer: Add terse documentation of assumed C implementation defined behavior
Suggested-by: "Ronald S. Bultje"
Signed-off-by: Michael Niedermayer
---
doc/developer.texi | 5 +
1 file changed, 5 insertions(+)
diff --git a/doc/developer.texi b/doc/developer.texi
index dbe1f5421f..a948113792 100644
--- a/doc/developer.texi
+++ b/doc/developer.texi
@@ -131,6 +131,11 @@ designated struct initializers (@samp{struct s x = @{ .i =
17 @};});
@item
compound literals (@samp{x = (struct s) @{ 17, 23 @};}).
+
+@item
+Implementation defined behavior for signed integers is assumed to match the
+expected for Twos complement. Non representable values in integer casts are
binary
+truncated. Shift right of signed values uses sign extension.
@end itemize
These features are supported by all compilers we care about, so we will not
--
2.11.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/pthread_frame, decode: allow errors to happen on draining
On Fri, Apr 28, 2017 at 01:32:04AM +0700, Muhammad Faiz wrote: > So, all frames and errors are correctly reported in order. > Fix fate failure with THREADS>=4: > make fate-h264-attachment-631 THREADS=4 > > Suggested-by: wm4, Ronald S. Bultje, Marton Balint > Signed-off-by: Muhammad Faiz > --- > libavcodec/decode.c| 3 ++- > libavcodec/pthread_frame.c | 15 +++ > 2 files changed, 9 insertions(+), 9 deletions(-) This breaks with opus and shorten, ill send you the sample files privatly [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/concatdec: port to the new bitstream filter API
On Wed, Apr 26, 2017 at 04:40:55PM -0300, James Almer wrote: > Signed-off-by: James Almer > --- > libavformat/concatdec.c | 86 > +++-- > 1 file changed, 26 insertions(+), 60 deletions(-) breaks ./ffmpeg -f concat -i ~/tickets/3108/concatfile.txt -codec copy test.avi (output produces many warnings/errors on playback) https://trac.ffmpeg.org/raw-attachment/ticket/3108/examplefiles.zip [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: hold old key info when append list
2017-04-26 7:30 GMT+08:00 Steven Liu :
> fix ticket id: #6353
>
> Signed-off-by: Steven Liu
> ---
> libavformat/hlsenc.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 27c8e3355d..3ec0f330fb 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -810,6 +810,7 @@ static int parse_playlist(AVFormatContext *s, const
> char *url)
> int64_t new_start_pos;
> char line[1024];
> const char *ptr;
> +const char *end;
>
> if ((ret = ffio_open_whitelist(&in, url, AVIO_FLAG_READ,
> &s->interrupt_callback, NULL,
> @@ -842,6 +843,29 @@ static int parse_playlist(AVFormatContext *s, const
> char *url)
> } else if (av_strstart(line, "#EXTINF:", &ptr)) {
> is_segment = 1;
> hls->duration = atof(ptr);
> +} else if (av_stristart(line, "#EXT-X-KEY:", &ptr)) {
> +ptr = av_stristr(line, "URI=\"");
> +if (ptr) {
> +ptr += strlen("URI=\"");
> +end = av_stristr(ptr, ",");
> +if (end) {
> +av_strlcpy(hls->key_uri, ptr, end - ptr);
> +} else {
> +av_strlcpy(hls->key_uri, ptr, sizeof(hls->key_uri));
> +}
> +}
> +
> +ptr = av_stristr(line, "IV=0x");
> +if (ptr) {
> +ptr += strlen("IV=0x");
> +end = av_stristr(ptr, ",");
> +if (end) {
> +av_strlcpy(hls->iv_string, ptr, end - ptr);
> +} else {
> +av_strlcpy(hls->iv_string, ptr,
> sizeof(hls->iv_string));
> +}
> +}
> +
> } else if (av_strstart(line, "#", NULL)) {
> continue;
> } else if (line[0]) {
> --
> 2.11.0 (Apple Git-81)
>
>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Applied!
Thanks
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix CID 1405135
2017-04-26 16:43 GMT+08:00 Steven Liu :
> Fixes Coverity CID: 1405135
>
> Signed-off-by: Steven Liu
> ---
> libavformat/hlsenc.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 3ec0f330fb..b7aafb73da 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -394,11 +394,11 @@ static int do_encrypt(AVFormatContext *s)
> av_strlcat(hls->key_basename, ".key", len);
>
> if (hls->key_url) {
> -strncpy(hls->key_file, hls->key_url, sizeof(hls->key_file));
> -strncpy(hls->key_uri, hls->key_url, sizeof(hls->key_uri));
> +av_strlcpy(hls->key_file, hls->key_url, sizeof(hls->key_file));
> +av_strlcpy(hls->key_uri, hls->key_url, sizeof(hls->key_uri));
> } else {
> -strncpy(hls->key_file, hls->key_basename, sizeof(hls->key_file));
> -strncpy(hls->key_uri, hls->key_basename, sizeof(hls->key_uri));
> +av_strlcpy(hls->key_file, hls->key_basename,
> sizeof(hls->key_file));
> +av_strlcpy(hls->key_uri, hls->key_basename, sizeof(hls->key_uri));
> }
>
> if (!*hls->iv_string) {
> --
> 2.11.0 (Apple Git-81)
>
>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Applied!
Thanks
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
