[MPlayer-dev-eng] [PATCH 2/2] av_sub: support multiple rectangles.

[Reimar Döffinger]

On Sun, Apr 22, 2012 at 06:32:16PM +0200, Nicolas George wrote:
> +    int i, xmin = INT_MAX, ymin = INT_MAX, xmax = INT_MIN, ymax = INT_MIN;

I don't think we want to allow max values < 0?

> +    if (num_rects == 1) {

Is num_rects == 0 handled somewhere? It seems that case would badly
upset both code paths.

> @@ -1376,27 +1363,70 @@ void spudec_set_paletted(void *this, const uint8_t *pal_img, int pal_stride,
>    packet->start_col = x;
>    packet->start_row = y;
>    packet->data_len = 2 * stride * h;
> -  if (packet->data_len) { // size 0 is a special "clear" packet
> -      packet->packet = malloc(packet->data_len);
> -      img  = packet->packet;
> -      aimg = packet->packet + stride * h;
> -      for (i = 0; i < 256; i++) {
> -          uint32_t pixel = pal[i];
> -          int alpha = pixel >> 24;
> -          int gray = (((pixel & 0x000000ff) >>  0) +
> -                      ((pixel & 0x0000ff00) >>  7) +
> -                      ((pixel & 0x00ff0000) >> 16)) >> 2;
> -          gray = FFMIN(gray, alpha);
> -          g8a8_pal[i] = (-alpha << 8) | gray;
> -      }
> -      pal2gray_alpha(g8a8_pal, pal_img, pal_stride,
> -                     img, aimg, stride, w, h);
> +  if (packet->data_len) {

Why did you remove the comment? That seems to be the only thing that
changed in that line.

> +    packet->packet = malloc(packet->data_len);
> +    if (!packet->packet) {
> +      free(packet->packet);

Quite pointless to free it when it is NULL...

> +      free(packet);
> +      return NULL;
> +    }
>    }
> +  return packet;

If it's trivial like here, I prefer using the same return path, by just
setting packet to NULL.

> +void spudec_packet_send(void *this, packet_t *packet, double pts, double endpts)
> +{
>    packet->start_pts = 0;
>    packet->end_pts = 0x7fffffff;
>    if (pts != MP_NOPTS_VALUE)
>      packet->start_pts = pts * 90000;
>    if (endpts != MP_NOPTS_VALUE)
>      packet->end_pts = endpts * 90000;
> -  spudec_queue_packet(spu, packet);
> +  spudec_queue_packet(this, packet);

The old code used "spu" on purpose, I'd prefer if you'd not use "this"
in any new code.
While it does not matter for us really, I don't see a point in making
code incompatible with C++ when it in addition is a horrible name
anyway.
I must say I am quite split.
It is a really nice and simple change, so I well can't say no to it
(except for those few minor issues).
On the other hand for large resolutions it might waste a lot of memory
and processing power, and I would have preferred to see an
implementation that just uses multiple subtitle processors...
Have you though about that way of doing it? I suspect it would be
more complex and problematic to support an arbitrary number of subs,
still I quite liked that idea...

Reimar