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

[Michael Niedermayer]

On Tue, Jan 14, 2014 at 08:33:20PM +0000, Rl wrote:
> 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.

should be ok then


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

its very easy just see for example:
ccb212b6c3ed18c9ff4e0c982574c43f92657f9f
and make sure the first element of the context is a AVClass



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

yes


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

well, then just drop the supported_framerates list and document the
possible need for 30fps for compatibility purposes

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140115/4f9a12d5/attachment.asc>