On date Friday 2011-06-03 01:22:39 +0200, Stefano Sabatini encoded:
> On date Thursday 2011-06-02 20:00:45 +0200, Anton Khirnov encoded:
> > ---
> > libavdevice/bktr.c | 24 +++++++++++++-----------
> > 1 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/libavdevice/bktr.c b/libavdevice/bktr.c
> > index e8ff557..8e3b271 100644
> > --- a/libavdevice/bktr.c
> > +++ b/libavdevice/bktr.c
> > @@ -54,11 +54,10 @@ typedef struct {
> > int video_fd;
> > int tuner_fd;
> > int width, height;
> > - int frame_rate;
> > - int frame_rate_base;
> > uint64_t per_frame;
> > int standard;
> > char *video_size; /**< String describing video size, set by a private
> > option. */
> > + char *framerate; /**< Set by a private option. */
> > } VideoData;
> >
> >
> > @@ -249,8 +248,7 @@ static int grab_read_header(AVFormatContext *s1,
> > AVFormatParameters *ap)
> > VideoData *s = s1->priv_data;
> > AVStream *st;
> > int width, height;
> > - int frame_rate;
> > - int frame_rate_base;
> > + AVRational fps;
> I have the already noted nit, fps may be replaced by framerate_q but
> leave this if you don't like it for whatever reason, it's just an
> internal variable and can be easily changed later.
And now that I think at it, AVRational framerate would be even better,
since there is no possibility of conflict with s1->framerate in this
context.
>
> > int ret = 0;
> >
> > if (ap->time_base.den <= 0) {
> > @@ -262,14 +260,18 @@ static int grab_read_header(AVFormatContext *s1,
> > AVFormatParameters *ap)
> > av_log(s1, AV_LOG_ERROR, "Couldn't parse video size.\n");
> > goto out;
> > }
> > + if ((ret = av_parse_video_rate(&fps, s->framerate)) < 0) {
> > + av_log(s1, AV_LOG_ERROR, "Couldn't parse framerate.\n");
> > + goto out;
> > + }
Usability nit:
"Couldn't parse framerate '%s'.\n"
is usually more useful ("show the user where the error is" criteria),
and I believe that Diego favors not-abbreviated expressions (Couldn't
-> Could not).
[...]
Rest looks good to me.
--
Misfortunes arrive on wings and leave on foot.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
