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

[wm4]

On Sun, 13 Jul 2014 22:17:44 +0200
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> 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.

No. It just happened to work with the PNG format. In general, setting
the dimensions before opening was required. When the error check was
added, someone noticed that it broke mplayer, and added this
"exception".

> 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.

Haha. No. Encoding a screenshot takes far longer. And the mplayer video
chain even requires explicit reconfiguration on video size change, so
what the heck?

> 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).