[FFmpeg-devel] Another possible bug in dvbsub.c

[Ronen Mizrahi]

>
>
>> -    for (region_id = 0; region_id < h->num_rects; region_id++) {
>> -        *q++ = region_id;
>> -        *q++ = 0xff; /* reserved */
>> -        bytestream_put_be16(&q, h->rects[region_id]->x); /* left pos */
>> -        bytestream_put_be16(&q, h->rects[region_id]->y); /* top pos */
>> +    if (!s->hide_state) {
>> +        for (region_id = 0; region_id < h->num_rects; region_id++) {
>> +            *q++ = region_id;
>> +            *q++ = 0xff; /* reserved */
>> +            bytestream_put_be16(&q, h->rects[region_id]->x); /* left pos
>> */
>> +            bytestream_put_be16(&q, h->rects[region_id]->y); /* top pos
>> */
>> +        }
>>     }
>>
>
> Indent should be done in a separate patch. That way it's easier so see that
> this one doesn't change the behavior of the loop.
>

There is no indentation for the sake of indentation, an if statement is now
enclosing the loop and that is why it is indented.


>
>      bytestream_put_be16(&pseg_len, q - pseg_len - 2);
>>
>>     if (!s->hide_state) {
>> +        for (region_id = 0; region_id < h->num_rects; region_id++) {
>> +
>> +            /* region composition segment */
>> +
>> +            if (h->rects[region_id]->nb_colors <= 4) {
>> +                /* 2 bpp, some decoders do not support it correctly */
>> +                bpp_index = 0;
>> +            } else if (h->rects[region_id]->nb_colors <= 16) {
>> +                /* 4 bpp, standard encoding */
>> +                bpp_index = 1;
>> +            } else {
>> +                return -1;
>> +            }
>> +
>> +            *q++ = 0x0f; /* sync_byte */
>> +            *q++ = 0x11; /* segment_type */
>> +            bytestream_put_be16(&q, page_id);
>> +            pseg_len = q;
>> +            q += 2; /* segment length */
>> +            *q++ = region_id;
>> +            *q++ = (s->object_version << 4) | (0 << 3) | 0x07; /* version
>> , no fill */
>> +            bytestream_put_be16(&q, h->rects[region_id]->w); /* region
>> width */
>> +            bytestream_put_be16(&q, h->rects[region_id]->h); /* region
>> height */
>> +            *q++ = ((1 + bpp_index) << 5) | ((1 + bpp_index) << 2) |
>> 0x03;
>> +            *q++ = region_id; /* clut_id == region_id */
>> +            *q++ = 0; /* 8 bit fill colors */
>> +            *q++ = 0x03; /* 4 bit and 2 bit fill colors */
>> +
>> +            if (!s->hide_state) {
>>
>
> This check can be dropped since you're already doing it outside the loop.
>
> You are correct, I will submit a patch without this if statement.

Overall it looks OK. I'll take your word on it following the spec,
> especially if several STBs take the output.


Thank you.

So, will a patch without that if statement be accepted, or do you need me to
somehow break it to several patches? (I may be misunderstanding your comment
about indentation so please let me know if this is still an issue)

Best,

Ronen