[MPlayer-dev-eng] New remove-logo filter

[Yartrebo]

On Wed, 2005-02-23 at 12:18 +0100, Reimar Döffinger wrote:
> Hi,
> On Wed, Feb 23, 2005 at 01:55:21AM -0500, Yartrebo wrote:
> > Hi, and sorry, but I forgot one of the files in the diff patch. This one
> > goes into libmpcodecs.
> 
> I really think you should simplify the code a lot. First of all I think
> you should use raw grayscale instead of tga image, reading it in should
> the need no more that around 6 lines.

The image is made using a graphics program, such as the GIMP. Asking for
raw greyscale might be asking a bit too much. Internally it's stored
using 8bpp, so all that's lost is some hard disk space. Also, having the
width and height stored is very useful to make sure the image matches
the video stream dimensions (that check will be in the next revision I
submit).

TGA is one of the simplest formats that doesn't suffer for the problems
of raw data, so that's why I chose it. I could make the reader take 8-10
lines of code, but then it wouldn't be safe if an unsupported type (say,
RLE compressed) one was fed in.

If you have a way of making raw greyscale work with The GIMP and your
average graphics suite, and make it store the width and height, then
I'll be more open to using it.
> 
> 
> > /* This struct is from the example filter I used. As can be seen, our filter only supports a single format - YV12. */
> > static unsigned int bgr_list[]=
> > {
> >   IMGFMT_YV12,
> >   0
> > };
> > 
> > /* Likewise, this function is from the example filter I used. Not exactly sure how it works, but it works. */
> > static unsigned int find_best(struct vf_instance_s* vf){
> >     unsigned int best=0;
> >     int ret;
> >     unsigned int * p=bgr_list;
> >     while(*p){
> > 	ret=vf->next->query_format(vf->next,*p);
> > 	mp_msg(MSGT_VFILTER,MSGL_V,"[%s] query(%s) -> %d\n",vf->info->name,vo_format_name(*p),ret&3);
> > 	if(ret&VFCAP_CSP_SUPPORTED_BY_HW){ best=*p; break;} // no conversion -> bingo!
> > 	if(ret&VFCAP_CSP_SUPPORTED && !best) best=*p; // best with conversion
> > 	++p;
> >     }
> >     return best;
> > }
> 
> Since you support only YV12, remove these. And I guess you can remove
> the fmt entry from the vf_priv_s struct.
Sure. I have little idea how they work, so I was afraid to touch them.
I'll remove them.
> 
> > /* Once again, this function is only slightly modified from the example filter's version. Not exactly sure how it works either. */
> > static int config(struct vf_instance_s* vf,
> >         int width, int height, int d_width, int d_height,
> > 	unsigned int flags, unsigned int outfmt){
> >     if (!((vf_priv_s *)vf->priv)->fmt)
> > 	((vf_priv_s *)vf->priv)->fmt=find_best(vf);
> >     if(!((vf_priv_s *)vf->priv)->fmt){
> > 	// no matching fmt, so force one...
> > 	if(outfmt==IMGFMT_RGB8) ((vf_priv_s *)vf->priv)->fmt=IMGFMT_RGB32;
> > 	else if(outfmt==IMGFMT_BGR8) ((vf_priv_s *)vf->priv)->fmt=IMGFMT_BGR32;
> > 	else return 0;
> >     }
> >     return vf_next_config(vf,width,height,d_width,d_height,flags,((vf_priv_s *)vf->priv)->fmt);
> > }
> 
> Leave only return
> vf_next_config(vf,width,height,d_width,d_height,flags,IMGFMT_YV12);
Sure
> 
> > static void convert_yv12(vf_instance_t * vf, mp_image_t * mpi, mp_image_t * dmpi)
> 
> This needs some optimization IMHO...
> First, I think the code for chrominance and luminance is mostly the
> same, maybe put it in a separate function.
> Next, you shouldn't do the copying here - especially as it isn't
> necessary to do it at all when the MPI_IMGFLAG_DIRECT is set.
> When it is not set you should just use the memcpy_pic function first IMHO (see also how the vf_delogo filter does it).
I found that one out the hard way this morning. Not only is it
inefficient when the source and image are the same, but the code messes
up too (you get all black, except for the logo region). That will
definitely be fixed.
> 
> > /* Once again, not sure how this works because it's from another filter. */
> > static int query_format(struct vf_instance_s * vf, unsigned int fmt){
> >     int best;
> >     best=find_best(vf);
> >     if(!best) return 0; // no match
> >     return vf->next->query_format(vf->next,best);
> > }
> 
> Just do
> if (fmt == IMGFMT_YV12)
>   return vf->next->query_format(vf->next, IMGFMT_YV12);
> return 0;
Sure. Thanks.
> 
> > /* Initialize our filter. */
> > static int open(vf_instance_t * vf, char* args){
> 
> Maybe add a if (!args) ... here and give a short help message, too.
> 
> >     vf->priv = safe_malloc(sizeof(vf_priv_s));
> > 
> >     /* Load our filter image. */
> >     if (args) ((vf_priv_s *)vf->priv)->filter = load_tga(args);
> >     if (((vf_priv_s *)vf->priv)->filter == NULL)
> >       return 0;
> 
> You would have to free vf->priv here...
Hmm, minor oversight. I guess I'll put it in.
> 
> Greetings,
> Reimar Döffinger
> 
I'm hoping to have the fixed version ready later today.

Thanks,
Robert Edele