[FFmpeg-devel] [RFC] ffplay dr1 + reget buffer issue

[Jai Menon]

On Sun, May 23, 2010 at 11:22 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, May 23, 2010 at 09:05:04PM +0530, Jai Menon wrote:
>> Hi,
>>
>> Here's yet another ffplay issue i thought i'd post. Currently, the
>> ffplay input filter doesn't define a reget_buffer callback for codecs.
>> So codecs fallback to avcodec_default_reget_buffer which always tries
>> to obtain a new buffer and copy the old picture onto it. This used to
>> work fine for codecs which output rgb earlier because internal buffers
>> are always "reused". With current ffplay however, buffer_type is
>> FF_BUFFER_TYPE_USER which means that with every reget_buffer call, we
>> unnecessarily make get/release buffer calls. Also, since
>> av_picture_copy doesn't account for the single data pointer used for
>> rgb pixfmts, all codecs using reget_buffer and outputting rgb crash
>> with ffplay. I'm not sure which "solution" is preferable here (or if
>> there is something else which can be done), so attached are two
>> patches for the different issues. both of them fix the issue
>> independently. comments welcome.
>
> i think we want both paches, comments below
>
>>
>> --
>> Jai Menon
>
>> ?ffplay.c | ? 12 ++++++++++++
>> ?1 file changed, 12 insertions(+)
>> c1749823e51b918e66af05b6635754f3fb86b0d3 ?ffplay-reget-buffer.diff
>> diff --git a/ffplay.c b/ffplay.c
>> index a48891e..d2f0e67 100644
>> --- a/ffplay.c
>> +++ b/ffplay.c
>> @@ -1607,6 +1607,17 @@ static void input_release_buffer(AVCodecContext *codec, AVFrame *pic)
>> ? ? ?avfilter_unref_pic(pic->opaque);
>> ?}
>>
>> +static int input_reget_buffer(AVCodecContext *codec, AVFrame *pic)
>> +{
>> + ? ?if(pic->data[0] == NULL) {
>> + ? ? ? ?pic->buffer_hints |= FF_BUFFER_HINTS_READABLE;
>> + ? ? ? ?return codec->get_buffer(codec, pic);
>> + ? ?}
>> +
>> + ? ?pic->reordered_opaque = codec->reordered_opaque;
>> + ? ?return 0;
>> +}
>
> iam not sure if we have to, but i think we should check that
> width/height/pixfmt matches

Would a check whether linesizes with current image properties (using
ff_fill_linesize on a throwaway pic) and the input AVFrame match, be
enough? I'm not sure if anything more than that required though (or
perhaps something the code in its current state supports).

>
> [...]
>
>> ?ffplay.c ? ? ? ? ? ? ? ?| ? ?2 ++
>> ?libavcodec/imgconvert.c | ? ?2 ++
>> ?2 files changed, 4 insertions(+)
>> e683c28d0ca25300e038279dbe37736a4a7614e6 ?picture-copy-rgb-fix.diff
>> diff --git a/ffplay.c b/ffplay.c
>> index a48891e..7b20848 100644
>> --- a/ffplay.c
>> +++ b/ffplay.c
>> @@ -1591,7 +1591,9 @@ static int input_get_buffer(AVCodecContext *codec, AVFrame *pic)
>> ? ? ? ? ?unsigned hshift = i == 0 ? 0 : av_pix_fmt_descriptors[ref->pic->format].log2_chroma_w;
>> ? ? ? ? ?unsigned vshift = i == 0 ? 0 : av_pix_fmt_descriptors[ref->pic->format].log2_chroma_h;
>>
>> + ? ? ? ?if (ref->data[i]) {
>> ? ? ? ? ?ref->data[i] ? ?+= (edge >> hshift) + ((edge * ref->linesize[i]) >> vshift);
>> + ? ? ? ?}
>> ? ? ? ? ?pic->data[i] ? ? = ref->data[i];
>> ? ? ? ? ?pic->linesize[i] = ref->linesize[i];
>> ? ? ?}
>
> hunk ok, feel free to commit

applied.

>> diff --git a/libavcodec/imgconvert.c b/libavcodec/imgconvert.c
>> index 8f789c4..a00ab56 100644
>> --- a/libavcodec/imgconvert.c
>> +++ b/libavcodec/imgconvert.c
>> @@ -979,9 +979,11 @@ void av_picture_copy(AVPicture *dst, const AVPicture *src,
>> ? ? ? ? ? ? ?if (i == 1 || i == 2) {
>> ? ? ? ? ? ? ? ? ?h= -((-height)>>desc->log2_chroma_h);
>> ? ? ? ? ? ? ?}
>> + ? ? ? ? ? ?if (dst->data[i]) {
>> ? ? ? ? ? ? ?ff_img_copy_plane(dst->data[i], dst->linesize[i],
>
> the first line in ff_img_copy_plane() is
> ? ?if((!dst) || (!src))
> ? ? ? ?return;

err, sorry for that. i didn't intend for that hunk to be included.

-- 
Jai Menon