[FFmpeg-devel] [PATCH][RFC] AVFMT_RAWPICTURE, null format and 2-pass log

[Stefano Sabatini]

On date Sunday 2008-08-10 01:17:16 +0200, Michael Niedermayer encoded:
> On Sat, Aug 09, 2008 at 11:33:44AM +0200, Stefano Sabatini wrote:
> > Hi all,
> > 
> > I see no reason for which when is
> > s->oformat->flags & AVFMT_RAWPICTURE
> > 
> > ffmpeg shouldn't print on the 2-passlog file.
> > 
> > Also I cannot understand why the null_muxer is configured with the
> > AVFMT_RAWPICTURE flag, which I'd expect instead in rawvideo_muxer (where
> > it is missing).
> > 
> > Are the attached patches OK, or would you like to comment on the
> > meaning of the AVFMT_RAWPICTURE flag?
> > 
> > Current documentation says:
> > #define AVFMT_RAWPICTURE    0x0020 /**< format wants AVPicture structure for
> >                                       raw picture data */
> > 
> > BTW each one of the patch fixes the problem for which if I do:
> > 
> > ffmpeg -i ~/test.mov -vcodec libxvid -pass 1 -passlogfile test1 -an -f null -y /dev/null
> > 
> > then the pass-logfile issued is empty.
> > 
> > Regards.
> > -- 
> > FFmpeg = Forgiving Fostering Multimedia Portable Elastic Gymnast
> 
> > Index: libavformat/raw.c
> > ===================================================================
> > --- libavformat/raw.c	(revision 14673)
> > +++ libavformat/raw.c	(working copy)
> > @@ -850,7 +850,7 @@
> >      CODEC_ID_RAWVIDEO,
> >      NULL,
> >      null_write_packet,
> > -    .flags = AVFMT_NOFILE | AVFMT_RAWPICTURE | AVFMT_NOTIMESTAMPS,
> > +    .flags = AVFMT_NOFILE | AVFMT_NOTIMESTAMPS,
> >  };
> >  #endif //CONFIG_MUXERS
> >  
> 
> not ok, this makes the code do extra computations IIRC

I checked the code, this flag is used in the code only in ffmpeg.c,
and it avoids the encoding of the output frame in the below block.

The logic is that if we don't have to output the frame, then there is
no need to encode it, the outshoot of this is that this can't work in
this scenario, when we need to encode in order to fill the
enc->stats_out field of the encoder context.

So when it saves somehow some computation, it simply doesn't do the
right thing at least in this case.

So I'm still of the idea that this flag shouldn't be set here, but
only in those containers which can only contain rawvideo formats, such
as for YUV4MPEG muxer (currently also the only one).

> > Index: ffmpeg.c
> > ===================================================================
> > --- ffmpeg.c	(revision 14673)
> > +++ ffmpeg.c	(working copy)
> > @@ -922,6 +922,7 @@
> >  
> >              write_frame(s, &pkt, ost->st->codec, bitstream_filters[ost->file_index][pkt.stream_index]);
> >              enc->coded_frame = old_frame;
> > +            ret = 1;
> >          } else {
> >              AVFrame big_picture;
> >  
> > @@ -973,12 +974,14 @@
> >                  //fprintf(stderr,"\nFrame: %3d %3d size: %5d type: %d",
> >                  //        enc->frame_number-1, enc->real_pict_num, ret,
> >                  //        enc->pict_type);
> > -                /* if two pass, output log */
> > -                if (ost->logfile && enc->stats_out) {
> > -                    fprintf(ost->logfile, "%s", enc->stats_out);
> > -                }
> >              }
> >          }
> > +
> > +        /* if two pass, output log */
> > +        if (ret>0 && ost->logfile && enc->stats_out) {
> > +            fprintf(ost->logfile, "%s", enc->stats_out);
> > +        }
> > +
> >          ost->sync_opts++;
> >          ost->frame_number++;
> >      }
> 
> ok if it fixes the bug and passes regressions

And as I discovered this patch alone doesn't fix the bug,
enc->stats_out is filled by the encoder so we need to encode to fill
it, so I think there is no point into applying it.

Regards.
-- 
FFmpeg = Faboulous and Fiendish MultiPurpose Erroneous God