[MPlayer-dev-eng] [PATCH] new vf_ass implementation

[Xidorn Quan]

On Thu, Oct 11, 2012 at 10:40 AM, Zongyao Qu <zongyao.qu at gmail.com> wrote:

> I am reviewing this as far as I could...
>

Thx for your reviewing.


> >  static int config(struct vf_instance *vf,
> >                    int width, int height, int d_width, int d_height,
> >                    unsigned int flags, unsigned int outfmt)
> >  {
> >      struct mp_eosd_settings res = {0};
> > +    struct dirty_rows_extent *dirty_rows;
> > +    int outw, outh;
> > +    int planes, alphas;
> > +    int i;
>
> >      vf->priv->outfmt = outfmt;
> > -    vf->priv->outh = height + ass_top_margin + ass_bottom_margin;
> > -    vf->priv->outw = width;
> > +    vf->priv->outh = outh = height + ass_top_margin + ass_bottom_margin;
> > +    vf->priv->outw = outw = width;
> >
> > -    if (!vf->priv->is_planar)
> > -        vf->priv->planes[0] = malloc(vf->priv->outw * vf->priv->outh);
> > -    vf->priv->planes[1]  = malloc(vf->priv->outw * vf->priv->outh);
> > -    vf->priv->planes[2]  = malloc(vf->priv->outw * vf->priv->outh);
> > -    vf->priv->dirty_rows = malloc(vf->priv->outh);
> > +    for (i = 0; i < planes; i++)
> > +        vf->priv->planes[i] = malloc(outw * outh);
> > +    for (i = 0; i < alphas; i++)
> > +        vf->priv->alphas[i] = malloc(outw * outh);
> > +    dirty_rows = malloc(outh * sizeof(*dirty_rows));
> > +    for (i = 0; i < outh; i++) {
> > +        dirty_rows[i].xmin = 0;
> > +        dirty_rows[i].xmax = outw;
> > +    }
>
> 1.
> what is the purpose of the local variables outw/outh in config()?
> why not using vf->priv->outh/outw directly?
>

To simplify code and accelerate a little for that compilers can
realize they are loop invariants, especially for the last loop.
Using vf->priv->outh/outw directly is ok as well since the loop isn't
very large and is only executed once.


> 2.
> The other planes/alphas pointers which are not used, better set to NULL.
> This is the only place to initialize the vf_priv_s, I am not sure
> in all platforms they are initialized to NULL, otherwise it will crash
> in uninit().
>
> Or alternatively, in m_struct.c:60, the vf->priv is initialized once
> by copied from vf_priv_dflt, so as a better solution, we could add the
> initial value to vf_priv_dflt which is located in vf_ass.c:74
>

According to C standard ISO/IEC 9989, 6.7.8, static-storage variables
which is not initialized explicitly should be initialized to zero.
So we don't need to worry about it.


> >  static int put_image(struct vf_instance *vf, mp_image_t *mpi, double
> pts)
> >  {
> >      struct mp_eosd_image_list images;
> >      eosd_render_frame(pts, &images);
> >      prepare_image(vf, mpi);
> > -    render_frame(vf, mpi, &images);
> > +    if (images.changed)
>                   ^^^^^^^
> Is there any docs you could find on how this flag works?
> I know nothing about it.
>

No docs, only code in function eosd_render_frame (sub/eosd.c) shows
that it will be non-zero if subtitles are changed. Maybe we'd better
append some docs for sub/eosd.h to clarify this field?


> > +        prepare_eosd(vf, &images);
> > +    vf->priv->render_frame(vf);
> >      return vf_next_put_image(vf, vf->dmpi, pts);
> >  }
> >
>