I've like to submit my remove_logo filter for filtering out tv logos. It works by using a bitmap (.tga format) to know which pixels are in the logo and which aren't. It then blurs out the tv logo, filling it in with colors from neighboring (and out of the logo) pixels. By using a circular blur instead of bi-linear interpolation and by using an image instead of a rectangle, the logo can be removed with much less blur than the existing delogo filter. A much more detailed description can be found in the source code (libmpcodecs/vf_remove_logo.c). Comments and constructive criticism would be appreciated. Sincerely, Robert Edele
Hi, and sorry, but I forgot one of the files in the diff patch. This one goes into libmpcodecs. Sincerely, Robert Edele
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.
/* 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.
/* 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);
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).
/* 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;
/* 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... Greetings, Reimar Döffinger
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
On Wed, Feb 23, 2005 at 10:48:00AM -0500, Yartrebo wrote:
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).
i agree, raw grayscale is a stupid format. btw is there such thing as grayscale tga?
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.
what do you mean by "wouldn't be safe"? if it fails or exhibits strange behavior, that's fine. just document that it needs raw uncompressed tga. rich
Hi, On Wed, Feb 23, 2005 at 11:26:56AM -0500, D Richard Felker III wrote:
On Wed, Feb 23, 2005 at 10:48:00AM -0500, Yartrebo wrote:
On Wed, 2005-02-23 at 12:18 +0100, Reimar Döffinger wrote:
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).
i agree, raw grayscale is a stupid format. btw is there such thing as grayscale tga?
Well, then I would suggest raw pgm, it seems to me it has everything you want and the format looks very simple - also the data itself is exactly in the format you need it in memory later, so that you can read it directly in (which was my point against tga). And it is supported by almost any Linux/Unix program I think. Greetings, Reimar Döffinger
On Wed, 2005-02-23 at 17:33 +0100, Reimar Döffinger wrote:
Hi, On Wed, Feb 23, 2005 at 11:26:56AM -0500, D Richard Felker III wrote:
On Wed, Feb 23, 2005 at 10:48:00AM -0500, Yartrebo wrote:
On Wed, 2005-02-23 at 12:18 +0100, Reimar Döffinger wrote:
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).
i agree, raw grayscale is a stupid format. btw is there such thing as grayscale tga?
Well, then I would suggest raw pgm, it seems to me it has everything you want and the format looks very simple - also the data itself is exactly in the format you need it in memory later, so that you can read it directly in (which was my point against tga). And it is supported by almost any Linux/Unix program I think.
Greetings, Reimar Döffinger
I'm reading up on the raw PGM format and it is very ugly. It's an ASCII format and it allows whitespace, so I need to parse that out, and then ASCII numbers have to be converted to binary ones. ASCII isn't very nice because so many errors can happen. You can have values out of the valid range, non-numeric characters, and who knows what. As far as the data being the way I want it, it needs to be pre-processed since the program doesn't use the actual data, but rather the distance to the edge of the logo (calculated at init time using the loaded data). That and The GIMP doesn't seem to support PGM. Sincerely, Robert Edele
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
Hi, On Wed, Feb 23, 2005 at 11:58:21AM -0500, Yartrebo wrote:
I'm reading up on the raw PGM format and it is very ugly. It's an ASCII format and it allows whitespace, so I need to parse that out, and then ASCII numbers have to be converted to binary ones. ASCII isn't very nice because so many errors can happen. You can have values out of the valid range, non-numeric characters, and who knows what.
No it is not (that's why I said raw pgm, not ASCII pgm). Only the header is TEXT.
That and The GIMP doesn't seem to support PGM.
Yes it does. At least 2.2, that's why I suggested it. Greetings, Reimar Döffinger
Hi, On Wed, Feb 23, 2005 at 06:14:59PM +0100, Reimar Döffinger wrote:
No it is not (that's why I said raw pgm, not ASCII pgm). Only the header is TEXT.
If that still scares you, I attached a working parser for pgm file. Instead of the last three fgetc you should of course use fread to read the whole image data in a block. Greetings, Reimar Döffinger
Thank you very much. That's exactly what I was looking for. I'll have the updated filter ready when I have time, probably by friday at the latest and hopefully tonight. Thanks, Robert Edele On Wed, 2005-02-23 at 19:00 +0100, Reimar Döffinger wrote:
Hi, On Wed, Feb 23, 2005 at 06:14:59PM +0100, Reimar Döffinger wrote:
No it is not (that's why I said raw pgm, not ASCII pgm). Only the header is TEXT.
If that still scares you, I attached a working parser for pgm file. Instead of the last three fgetc you should of course use fread to read the whole image data in a block.
Greetings, Reimar Döffinger _______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
Hi On Wednesday 23 February 2005 17:26, D Richard Felker III wrote:
On Wed, Feb 23, 2005 at 10:48:00AM -0500, Yartrebo wrote:
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).
i agree, raw grayscale is a stupid format. btw is there such thing as grayscale tga?
AFAIK yes, and theres also pgm [...] -- Michael "nothing is evil in the beginning. Even Sauron was not so." -- Elrond
Hi, I've revised my remove-logo filter to work using PGM/PPM files instead of TGA, and I've implemented most the suggested revisions. I'm now resubmitting my patch. As before, any comments or constructive criticism would be appreciated. Sincerely, Robert Edele
On Thursday 03 March 2005 07:49, Yartrebo wrote:
As before, any comments or constructive criticism would be appreciated.
+#define get_part_of_pixel_yv12(mpi, x, y, plane) (*(mpi->planes[(plane)] + (mpi->stride[(plane)] * (y)) + (x)))
You shouldn't be using multiplication in time critical areas. The whole point of stride, is that you have a variable which sits on the line you are currently at, and for each pixel you add 'x', and every line you add 'stride': uint8_t * tmp = dmpi->planes[0]; int x, y; for (y = 0; y < mpi->h; y++) { for (x = 0; x < mpi->w; x++) { do_stuff(tmp[j]); } tmp += dmpi->stride[0]; } Repeat for U and V. You should probably use a separate loop for them. (I made the same mistake BTW, even worse :) Other than that, patch looks good.. Haven't tested it though. BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt . - ods15
On Thu, Mar 03, 2005 at 01:35:47PM +0200, Oded Shimon wrote:
BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
Besides, they are mandatory now, so without them your patch is rejected. Diego
On Thu, Mar 03, 2005 at 12:48:22PM +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 01:35:47PM +0200, Oded Shimon wrote:
BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
Besides, they are mandatory now, so without them your patch is rejected.
Rejection rejected. We already discussed this on irc...his code is so excessively verbose that the LAST thing we want is more comments to bloat it up more! Besides, demanding code documentation for filters is nonsense. It's not like core components that have interdependence with other parts of mplayer. The filters are completely standalone modules that just conform to a particular api so mplayer can use them. Rich
Hi On Thursday 03 March 2005 17:41, D Richard Felker III wrote:
On Thu, Mar 03, 2005 at 12:48:22PM +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 01:35:47PM +0200, Oded Shimon wrote:
BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
Besides, they are mandatory now, so without them your patch is rejected.
Rejection rejected. We already discussed this on irc...his code is so excessively verbose that the LAST thing we want is more comments to bloat it up more! Besides, demanding code documentation for filters is nonsense. It's
IMHO the comments which are already there really should be changed to conform to the doxygen syntax [...] -- Michael "nothing is evil in the beginning. Even Sauron was not so." -- Elrond
On Thu, Mar 03, 2005 at 07:06:24PM +0100, Michael Niedermayer wrote:
Hi
On Thursday 03 March 2005 17:41, D Richard Felker III wrote:
On Thu, Mar 03, 2005 at 12:48:22PM +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 01:35:47PM +0200, Oded Shimon wrote:
BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
Besides, they are mandatory now, so without them your patch is rejected.
Rejection rejected. We already discussed this on irc...his code is so excessively verbose that the LAST thing we want is more comments to bloat it up more! Besides, demanding code documentation for filters is nonsense. It's
IMHO the comments which are already there really should be changed to conform to the doxygen syntax
Apparently this was not clear from my words, but this is what I meant: Existing comments should conform to doxygen syntax. Diego
On Thu, Mar 03, 2005 at 07:06:24PM +0100, Michael Niedermayer wrote:
Hi
On Thursday 03 March 2005 17:41, D Richard Felker III wrote:
On Thu, Mar 03, 2005 at 12:48:22PM +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 01:35:47PM +0200, Oded Shimon wrote:
BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
Besides, they are mandatory now, so without them your patch is rejected.
Rejection rejected. We already discussed this on irc...his code is so excessively verbose that the LAST thing we want is more comments to bloat it up more! Besides, demanding code documentation for filters is nonsense. It's
IMHO the comments which are already there really should be changed to conform to the doxygen syntax
OK, fair enough. He seems to like doxygen anyway. :) Rich
On Thu, 2005-03-03 at 14:36 -0500, D Richard Felker III wrote:
OK, fair enough. He seems to like doxygen anyway. :)
Rich
Hi, I've implemented the small change to save a few multiplications in my inner loop and I've made my comments Doxygen-friendly. The updated patches have been attached. As before, if you have any more suggestions, I'd be happy to hear them. Sincerely, Robert Edele
+.B remove-logo=/path/to/logo_bitmap_file_name.pgm +Suppresses a TV station logo using a PGM or PPM file to tell it
I'd remove this 'it'. Or maybe change to 'filter'
+which pixels comprise the logo. The width and height must match +those of the video stream being processed. Uses the filter +image and a circular blur algorithm to remove the logo.
Also please start each sentence on a new line. Torinthiel -- Waclaw "Torinthiel" Schiller GG#: 542916, 3073512 torinthiel(at)megapolis(dot)pl gpg: 0906A2CE fpr: EE3E DFB4 C4D6 E22E 8999 D714 7CEB CDDC 0906 A2CE "No classmates may be used during this examination"
Torinthiel wrote:
+.B remove-logo=/path/to/logo_bitmap_file_name.pgm +Suppresses a TV station logo using a PGM or PPM file to tell it
I'd remove this 'it'. Or maybe change to 'filter'
+which pixels comprise the logo. The width and height must match [snip]
"Use a PGM or PPM file to tell which pixels comprise the logo" is grammatically incorrect. "the filter" would be a more appropriate substitution. However, I think there's too much information in that one sentence. How about: "Suppresses TV station logos using a template. A PGM or PPM file must be supplied to indicate which pixels comprise the logo. The width and height ...." (and so on) Starting each sentence on a new line seems a little strange, but if you say so...
On Thu, Mar 10, 2005 at 11:55:16AM +0000, Martin Atkins wrote:
Starting each sentence on a new line seems a little strange, but if you say so...
That's a roff "oddity". The sentence delimiters in roff are either two spaces or a newline, so I chose a newline to settle on something. It's similar to LaTeX, where a period ends a sentence unless it is preceded by a capital letter, in which case you have to append a \ followed by a space or a newline to the period. Diego
Hi, For the sake of keeping an already hefty code base reasonable (my filter will become the second largest one if accepted), I don't think that three separate loops (requiring 3 implementations of get_blur(...) and 3 #defines) are a wise idea. However, that part about stride and multiplication are interesting, and I do access the entries sequentially. The speed-up will probably be marginal, but it will only take a few lines of code, so I guess I will implement that. Sincerely, Robert Edele On Thu, 2005-03-03 at 13:35 +0200, Oded Shimon wrote:
On Thursday 03 March 2005 07:49, Yartrebo wrote:
As before, any comments or constructive criticism would be appreciated.
+#define get_part_of_pixel_yv12(mpi, x, y, plane) (*(mpi->planes[(plane)] + (mpi->stride[(plane)] * (y)) + (x)))
You shouldn't be using multiplication in time critical areas. The whole point of stride, is that you have a variable which sits on the line you are currently at, and for each pixel you add 'x', and every line you add 'stride':
uint8_t * tmp = dmpi->planes[0]; int x, y; for (y = 0; y < mpi->h; y++) { for (x = 0; x < mpi->w; x++) { do_stuff(tmp[j]); } tmp += dmpi->stride[0]; }
Repeat for U and V. You should probably use a separate loop for them. (I made the same mistake BTW, even worse :)
Other than that, patch looks good.. Haven't tested it though. BTW, since you seen to like comments so much, you might want to use doxygen comments. They are pretty much the same as you have now, just built for doxygen... Check DOCS/tech/code-documentation.txt .
- ods15
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
Hi, I've noticed a minor bug in libvo/fastmemcpy.h. Apparently the const modifier was left out of: static inline void * memcpy_pic(void * dst, void * src, int bytesPerLine, int height, int dstStride, int srcStride) and static inline void * mem2agpcpy_pic(void * dst, void * src, int bytesPerLine, int height, int dstStride, int srcStride) They should read: static inline void * memcpy_pic(void * dst, const void * src, int bytesPerLine, int height, int dstStride, int srcStride) static inline void * mem2agpcpy_pic(void * dst, const void * src, int bytesPerLine, int height, int dstStride, int srcStride) I've attached a patch to this end. Sincerely, Robert Edele
On Thu, Mar 03, 2005 at 12:54:54AM -0500, Yartrebo wrote:
I've noticed a minor bug in libvo/fastmemcpy.h. Apparently the const modifier was left out of:
I've attached a patch to this end.
Please do not send patches buried in (random) unrelated threads. They are very likely to get overlooked this way. Diego
To what thread should I send them to then or how should I handle them? Robert Edele On Thu, 2005-03-03 at 12:14 +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 12:54:54AM -0500, Yartrebo wrote:
I've noticed a minor bug in libvo/fastmemcpy.h. Apparently the const modifier was left out of:
I've attached a patch to this end.
Please do not send patches buried in (random) unrelated threads. They are very likely to get overlooked this way.
Diego
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
On Thursday 03 March 2005 17:39, Yartrebo wrote:
To what thread should I send them to then or how should I handle them?
Create a new thread Regards, ismail
On Thu, Mar 03, 2005 at 10:39:32AM -0500, Yartrebo wrote:
To what thread should I send them to then or how should I handle them?
Compose a new message. Diego P.S.: Please don't top-post.
On Thu, 2005-03-03 at 12:14 +0100, Diego Biurrun wrote:
On Thu, Mar 03, 2005 at 12:54:54AM -0500, Yartrebo wrote:
I've noticed a minor bug in libvo/fastmemcpy.h. Apparently the const modifier was left out of:
I've attached a patch to this end.
Please do not send patches buried in (random) unrelated threads. They are very likely to get overlooked this way.
On Wed, 23 Feb 2005, 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.
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
On Wed, Feb 23, 2005 at 12:54:55PM -0800, Trent Piepho wrote:
On Wed, 23 Feb 2005, 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.
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
imo it's extra bloat and little point. we could also support all kinds of images... but unless/until mplayer has a common image-loader api, it's really ugly bloat to put all kinds of image-loaders (and all the #ifdefs needed to check for each crappy lib) in every single filter that needs to load an image file... rich
On Wed, 23 Feb 2005, D Richard Felker III wrote:
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
imo it's extra bloat and little point. we could also support all kinds
If mplayer already has libpng for other reasons, then you just have to compare the difference between the code to use it, vs the code for a pgm decoder and loader. It's not clear to me that using libpng would be more complex than writing a loader for pgm images. I suppose I can always write the code and see.
of images... but unless/until mplayer has a common image-loader api, it's really ugly bloat to put all kinds of image-loaders (and all the #ifdefs needed to check for each crappy lib) in every single filter that needs to load an image file...
Just make the entire filter depend on configure finding libpng, which it already looks for. Once you support png, there really isn't any point to supporting another format. The advantage of png is that you can use a compressed bitmap instead of an uncompressed greyscale image, and you could use png images as well for the alpha-blending filter. In fact, you could use the SAME png images for both filters.
On Wed, Feb 23, 2005 at 04:15:29PM -0800, Trent Piepho wrote:
On Wed, 23 Feb 2005, D Richard Felker III wrote:
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
imo it's extra bloat and little point. we could also support all kinds
If mplayer already has libpng for other reasons, then you just have to compare the difference between the code to use it, vs the code for a pgm decoder and loader. It's not clear to me that using libpng would be more complex than writing a loader for pgm images. I suppose I can always write the code and see.
it's much more complex. but the worst complexity of all is the extra conditional compiling crap, which makes the code unreadable.
of images... but unless/until mplayer has a common image-loader api, it's really ugly bloat to put all kinds of image-loaders (and all the #ifdefs needed to check for each crappy lib) in every single filter that needs to load an image file...
Just make the entire filter depend on configure finding libpng, which it already looks for.
no, this is stupid. i disable libpng because it's unneeded bloat, and i don't want this to disable useful filters too!!
Once you support png, there really isn't any point to supporting another format.
why? this could be said about any image format you're particular to, but it doesn't change the fact that someone else will have a different preference. rich
Trent Piepho wrote:
Just make the entire filter depend on configure finding libpng, which it already looks for. Once you support png, there really isn't any point to supporting another format. The advantage of png is that you can use a compressed bitmap instead of an uncompressed greyscale image, and you could use png images as well for the alpha-blending filter. In fact, you could use the SAME png images for both filters.
Indeed. vf_deblendlogo (currently also under discussion on the list) already ignores completely opaque pixels with the intention that they'd be handled by a blurring delogo filter instead. If this new filter looked for completely opaque pixels in the template and did the blur there exactly the same template could be used for both. There's already PNG-loading code that will require little modification in the deblendlogo filter. However, that entire filter is disabled if libpng isn't linked, which might be seen as a major drawback by some. A simple, generic image loading API for mplayer which supports all available image loaders would help fix this, but would be extra bloat in itself I suppose. In fact, if both filters are going to share the same template loading code and operate on the same template, it could be argued that they'd be better merged into one all-encompassing delogo filter which just takes a template and does the right thing for both transparent and [near-]opaque elements. Having both together might make for a bit of an unweildy filter as regards code readability, though. I dunno. All the best, Martin Atkins
Hi, On Fri, Feb 25, 2005 at 04:21:34PM +0000, Martin Atkins wrote:
There's already PNG-loading code that will require little modification in the deblendlogo filter. However, that entire filter is disabled if libpng isn't linked, which might be seen as a major drawback by some. A simple, generic image loading API for mplayer which supports all available image loaders would help fix this, but would be extra bloat in itself I suppose.
Look at my pgm-loading code in this thread, it can be use with almost zero modification for RGB ppm images, and it's less than 50 lines of pur C code, less (or almost) than your libpng interface code! Greetings, Reimar Döffinger
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
On Fri, Feb 25, 2005 at 04:21:34PM +0000, Martin Atkins wrote:
There's already PNG-loading code that will require little modification in the deblendlogo filter. However, that entire filter is disabled if libpng isn't linked, which might be seen as a major drawback by some. A simple, generic image loading API for mplayer which supports all available image loaders would help fix this, but would be extra bloat in itself I suppose.
Look at my pgm-loading code in this thread, it can be use with almost zero modification for RGB ppm images, and it's less than 50 lines of pur
But the deblendlogo filter uses an alpha channel, and ppm images don't support alpha.
On Friday 25 February 2005 20:08, Trent Piepho wrote:
On Fri, 25 Feb 2005, Reimar D�ffinger wrote:
On Fri, Feb 25, 2005 at 04:21:34PM +0000, Martin Atkins wrote:
There's already PNG-loading code that will require little modification in the deblendlogo filter. However, that entire filter is disabled if libpng isn't linked, which might be seen as a major drawback by some. A simple, generic image loading API for mplayer which supports all available image loaders would help fix this, but would be extra bloat in itself I suppose.
Look at my pgm-loading code in this thread, it can be use with almost zero modification for RGB ppm images, and it's less than 50 lines of pur
But the deblendlogo filter uses an alpha channel, and ppm images don't support alpha.
Yes, the 2 filters are quite different in what input they require - one needs only a bit a per pixel - blur? yes/no. the other needs RGB and A... It is still possible to make them both accept same files though... basically if A is nonzero, it means to the blurring 'remove_logo' that the area should be blurred.. that's all... PNG is probably not the best file format. does anyone have an idea for a good format which allows 4 channels?... - ods15
Hi, On Fri, Feb 25, 2005 at 10:08:17AM -0800, Trent Piepho wrote:
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
Look at my pgm-loading code in this thread, it can be use with almost zero modification for RGB ppm images, and it's less than 50 lines of pur
But the deblendlogo filter uses an alpha channel, and ppm images don't support alpha.
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round seems a bt more complicated. Also it is even more obvious how you can use the mask for the one for the other *g*. A separate alpha image might be better for the delogo filter speedwise, when most of the image doesn't contain the image - alpha only has a better chance to fit in cache. Greetings, Reimar Döffinger
On Fri, Feb 25, 2005 at 08:43:24PM +0100, Reimar Döffinger wrote:
Hi, On Fri, Feb 25, 2005 at 10:08:17AM -0800, Trent Piepho wrote:
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
Look at my pgm-loading code in this thread, it can be use with almost zero modification for RGB ppm images, and it's less than 50 lines of pur
But the deblendlogo filter uses an alpha channel, and ppm images don't support alpha.
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round seems a bt more complicated. Also it is even more obvious how you can use the mask for the one for the other *g*. A separate alpha image might be better for the delogo filter speedwise, when most of the image doesn't contain the image - alpha only has a better chance to fit in cache.
after loading the logo, the filters can find a bounding box for it and copy it to a smaller buffer with minimal stride. just make sure it stays 8-byte aligned for vector optimizations, though, when they're added in the future. rich
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round
Automatically with the gimp? My gimp doesn't have that option. I guess you have to extract the alpha somehow and save it separately, I'm not a gimp expert. I know as a user I'd rather deal with one compressed png image for each bug than multiple different format uncompressed images. Keep in mind, ATSC is up to 1920x1080 resolution. It seems that with ppm+pgm files, you're only making the code a little simpler if at all, but have something that's less useful to use.
On Saturday 26 February 2005 03:39, Trent Piepho wrote:
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round
Automatically with the gimp? My gimp doesn't have that option. I guess you have to extract the alpha somehow and save it separately, I'm not a gimp expert.
Remember, you don't start with a rgba png image. You start with a black frame with the logo on it; a screen capture. If you can make a rgba png from that, you can also make a ppm and pgm file from that. In fact, the latter is easier: Load the rgb screen capture Set mode to greyscale Save as pgm Close image Load the rgb screen capture Select all non-black pixels and fill with white Save as ppm If you use -vo pnm:pgm to capture the frame, it's even simpler. Anyway, I believe this is in the wrong thread. The remove_logo filter only needs an on/off bitmap (which, if i read the code correctly, can be the alpha channel; all non-zero pixels are treated as pixels to be blurred). But, the above procedure should be correct for the deblendlogo filter. So, both filters should read a pgm alpha channel and one of them (the deblend one) should also read a ppm. For collored logo's, the procedure is slightly different (only for deblend), but still not complicated. --Ivo
Trent Piepho wrote ( On Fri, Feb 25, 2005 at 06:39:32PM -0800 ):
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round
Automatically with the gimp? My gimp doesn't have that option. I guess you have to extract the alpha somehow and save it separately, I'm not a gimp expert.
I know as a user I'd rather deal with one compressed png image for each bug than multiple different format uncompressed images. Keep in mind, ATSC is up to 1920x1080 resolution. It seems that with ppm+pgm files, you're only making the code a little simpler if at all, but have something that's less useful to use.
IMHO png alone is not acceptable. As long as there is no common image loading facility provided by mplayer it is best to use simple formats like ppm/pgm or tga. Alex (beastd)
Hi On Saturday 26 February 2005 06:48, Alexander Strasser wrote:
Trent Piepho wrote ( On Fri, Feb 25, 2005 at 06:39:32PM -0800 ):
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round
Automatically with the gimp? My gimp doesn't have that option. I guess you have to extract the alpha somehow and save it separately, I'm not a gimp expert.
I know as a user I'd rather deal with one compressed png image for each bug than multiple different format uncompressed images. Keep in mind, ATSC is up to 1920x1080 resolution. It seems that with ppm+pgm files, you're only making the code a little simpler if at all, but have something that's less useful to use.
IMHO png alone is not acceptable. As long as there is no common image loading facility provided by mplayer it is best to use simple formats like ppm/pgm or tga.
why not use libavcodec/format for image loading? it supports png,ppm,pgm,pam,pbm,jpg,gif,raw, probably some obscure ones i forgot and practically every video codec missused as image coder too btw, see vhook/watermark.c for some well commented code which uses lav* for image loading (its a little complex as it needs demuxer+decoder init, someone should add some nicer and simpler interface for image loading ...) [...] -- Michael "nothing is evil in the beginning. Even Sauron was not so." -- Elrond
Hi, Michael Niedermayer wrote ( On Sat, Feb 26, 2005 at 12:13:21PM +0100 ):
IMHO png alone is not acceptable. As long as there is no common image loading facility provided by mplayer it is best to use simple formats like ppm/pgm or tga.
why not use libavcodec/format for image loading? it supports png,ppm,pgm,pam,pbm,jpg,gif,raw, probably some obscure ones i forgot and practically every video codec missused as image coder too
btw, see vhook/watermark.c for some well commented code which uses lav* for image loading (its a little complex as it needs demuxer+decoder init, someone should add some nicer and simpler interface for image loading ...)
Sounds good to me in general. Tho this would demand for a dependency on libav*. Also there are lots of other things that would demand for it. But esscpecially with libavformat we would have lot of duplicate functionality afaik. So there are some problems to be solved before this can happen. Ok, one could add an interface for image loading to mplayer and implement libav* support and a fallback to a simple format. But this would still suck as i think you would like to have the image loading interface for ffmpeg too. So best would be to merge the functionality differences there are beetween libavformat so it is all in one place... I hope you get what i mean. Sorry if i expressed it unclearly. Alex (beastd)
Is there any reason why I cannot support several formats? I could keep the tga loader and add in a pgm and bmp loader, and everyone would be happy. The loader to be used would be determined by the magic number (first 4 characters) of the file. Unless distribution size and compiled size is an issue, I don't see the cruft as being as big an issue as clueless (particularly MS Windows) users scratching their heads over what is a pgm or png file. The loader code can be relegated to the very end of the source file, seperated by 20 blank lines, if readability is a concern. Sincerely, Robert Edele On Sat, 2005-02-26 at 06:48 +0100, Alexander Strasser wrote:
Trent Piepho wrote ( On Fri, Feb 25, 2005 at 06:39:32PM -0800 ):
On Fri, 25 Feb 2005, Reimar Döffinger wrote:
ppm for color + pgm for alpha. And yes, it is possible to create this with gimp (or in the worst case with tifftopnm), the other way round
Automatically with the gimp? My gimp doesn't have that option. I guess you have to extract the alpha somehow and save it separately, I'm not a gimp expert.
I know as a user I'd rather deal with one compressed png image for each bug than multiple different format uncompressed images. Keep in mind, ATSC is up to 1920x1080 resolution. It seems that with ppm+pgm files, you're only making the code a little simpler if at all, but have something that's less useful to use.
IMHO png alone is not acceptable. As long as there is no common image loading facility provided by mplayer it is best to use simple formats like ppm/pgm or tga.
Alex (beastd)
_______________________________________________ MPlayer-dev-eng mailing list MPlayer-dev-eng@mplayerhq.hu http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
Yartrebo writes:
Unless distribution size and compiled size is an issue,
It is.
I don't see the cruft as being as big an issue as clueless (particularly MS Windows) users scratching their heads over what is a pgm or png file.
You can safely ignore that crowd, they don't use MPlayer, much less one of its filters to remove a logo. Diego
Yartrebo wrote:
Is there any reason why I cannot support several formats? I could keep the tga loader and add in a pgm and bmp loader, and everyone would be happy. The loader to be used would be determined by the magic number (first 4 characters) of the file.
When you start doing that, it's time to create a common image loader API that can be called by all filters. Having a single filter implement multiple loaders within itself would just be wrong. From discussion I read earlier but can't find now, it seems that mplayer already has the makings of such a thing but it depends on one of the optional libraries. For the common image loader API I think it'd be necessary to allow callers to specify limitations on what kinds of files they can support. For example, deblendlogo could only support image formats that support an alpha channel, so supplying a PPM file to it would be pointless. I imagine that with none of the optional image libraries compiled in, the image loader API would essentially just end up being a PPM/PGM loader.
On Wed, Feb 23, 2005 at 12:54:55PM -0800, Trent Piepho wrote:
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
Reading a pgm with just libc requires less lines of code than reading a png using libpng calls. -- Ville
On Thu, Feb 24, 2005 at 01:58:18AM +0200, Ville Saari wrote:
On Wed, Feb 23, 2005 at 12:54:55PM -0800, Trent Piepho wrote:
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
Reading a pgm with just libc requires less lines of code than reading a png using libpng calls.
hahahahah! so true, though! rich
Hi, On Wed, Feb 23, 2005 at 12:54:55PM -0800, Trent Piepho wrote:
On Wed, 23 Feb 2005, Reimar Döffinger wrote:
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.
Since mplayer is probably already linked with libpng, wouldn't it be easy to support png images?
Depending on libpng for something that has nothing to do with png and no real reason to need it is inacceptable MHO. Also, at least if I look at the code in the delogo filter and compare it with my code to read pgm (reading ppm would be 99% identical), I must say it definitly is not easier, not even less code. And size really isn't an argument, we're talking about ca. 3MB max (for color images). Greetings, Reimar Döffinger
participants (13)
-
Alexander Strasser -
D Richard Felker III -
Diego Biurrun -
Ismail Donmez -
Ivo -
Martin Atkins -
Michael Niedermayer -
Oded Shimon -
Reimar Döffinger -
Torinthiel -
Trent Piepho -
Ville Saari -
Yartrebo