[FFmpeg-devel] [PATCH] Revert "lavc/utils: Do not require dimensions for PNG."

[Reimar Döffinger]

On Sun, Jul 13, 2014 at 08:38:44PM +0200, wm4 wrote:
> On Sun, 13 Jul 2014 19:32:56 +0100
> Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:
> 
> > mplayer-specifc hacks should not be in our codebase. mplayer should fix
> > it's own code. It is not our responsibility to work around their broken
> > code.
> > 
> > This reverts commit e8e575633faf19711910cf9caf59f7db300a9ccd.
> > 
> > Signed-off-by: Derek Buitenhuis <derek.buitenhuis at gmail.com>
> > ---
> >  libavcodec/utils.c |    4 +---
> >  1 files changed, 1 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 9fa8e16..6a8992a 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -1533,9 +1533,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code
> >          } else if (avctx->channel_layout) {
> >              avctx->channels = av_get_channel_layout_nb_channels(avctx->channel_layout);
> >          }
> > -        if(avctx->codec_type == AVMEDIA_TYPE_VIDEO &&
> > -           avctx->codec_id != AV_CODEC_ID_PNG // For mplayer
> > -        ) {
> > +        if(avctx->codec_type == AVMEDIA_TYPE_VIDEO) {
> >              if (avctx->width <= 0 || avctx->height <= 0) {
> >                  av_log(avctx, AV_LOG_ERROR, "dimensions not set\n");
> >                  ret = AVERROR(EINVAL);
> 
> +1
> 
> The (removed) comment says it all.

Definitely not, it is in fact rather pointless.
I'll have to dig into it.

> A project specific hack for
> something that used the API incorrectly. I surely hope mplayer fixed
> this in the meantime on their side, they had time enough to do that.

Well, IMHO if this is a requirement that was added, I would say
it was an API change, and MPlayer might just be the first where
the API change was detected.
Though admittedly since the hack was only for PNG it smells like
something fairly silly.
I suspect it is in part there to work around a fairly specific issue:
For a screenshot filter the resolution might change any time,
and requiring a full reinitialization is a bit of a pain/performance
issue, particularly when we have no real reason to require it.
But yes, unless someone feels like coming up with a implementation
for such a "feature" removing it probably makes most sense (though I
still prefer such things to be done at version bumps).
I'll try to double-check and fix any issues there are (btw, if such hack
are added in the future it would be good to add a big ugly message to
make sure anyone affected actually fixes the issues instead of
forgetting about it).