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

[Michael Niedermayer]

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

[...]

>  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


> 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;
[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100523/57eadae5/attachment.pgp>