[FFmpeg-devel] [PATCH] ROQ encoder: remove redundant messages, reduce constraints

[Rl]

On Mon, Jan 13, 2014 at 08:31:59PM +0100, Michael Niedermayer wrote:
> > @@ -114,7 +114,7 @@ static void roqvideo_decode_frame(RoqContext *ri)
> >                          if(k & 0x02) y += 4;
> >  
> >                          if (bytestream2_tell(&ri->gb) >= chunk_start + chunk_size) {
> > -                            av_log(ri->avctx, AV_LOG_ERROR, "Input buffer too small\n");
> > +//                            av_log(ri->avctx, AV_LOG_ERROR, "Input buffer too small\n");
> >                              return;
> >                          }
> >                          if (vqflg_pos < 0) {
> 
> do you have roq files which cause these messages to be printed but
> which decode correctly ?

Oh sorry for including the decoder change into the encoder patch.
This was inappropriate.

I know I had good looking files which triggered the messages (it looks
like the format does not prevent having short chunks per se, which would
imply leaving out the update information for some subblocks, equivalent
to one or more skipped ending "skip" coding words) but I have no roq
files at hand right now.

In any way, the messages seem to be misleading as they indicate a
"short chunk" condition, not necessarily a "small buffer" one.

The change is otherwise purely cosmetic.

> a quake compatibility option like you suggest in the TODO would be
> much nicer than requiring the user to globally reduce quality to be
> compatibile with quake

Right, but the current workaround makes the quality inconsistent anyway,
reducing it for the rest of the file:

> -               "Warning, generated a frame too big (%d > 65535), "           
> -               "try using a smaller qscale value.\n",                        
 ...
> -        enc->lambda *= 1.5;                                                  
 ...
> -        goto retry_encode;

so I do not think the change leads to a remarkable loss of functionality.

I am not familiar with codec option handling so tried to minimize the
effort necessary to make the encoder usable for higher quality video
encoding (not letting one certain/buggy decoder rule out everything
better).

> > -    .supported_framerates = (const AVRational[]){ {30,1}, {0,0} },
> > +/*     .supported_framerates = (const AVRational[]){ {30,1}, {0,0} }, */

> if its unneeded  then it should be completely removed not just
> commented out
> or maybe if its needed for the official decoders then a new
> AVCodec struct could be added without this restriction
> or AVCodecContext.profile could be used to select which restrictions
> apply

I can not tell which decoders are "official" nor can test them thus can
not answer this question :(
Yes, I guess that encoding with anything else than 30fps is quite
certainly incompatible with at least some decoders. On the other side,
this is a documentation issue - how to use the encoder "compatibly".
I suppose it is good if it is also usable "incompatibly" when this
is desirable.

I did not look before at how profiles are handled in ffmpeg and codec
option handling feels unclear for me as well, that's why I made the
changes in the most concise way which seemed to add some value.

(Unfortunately more work is necessary to make the encoder flexible enough
to approach the "general purpose" level of usability. I do not currently
have the resources for this work but the demand if any is probably very
low.)

Regards,
Rl