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

[Tomas Härdin]

Ronen Mizrahi skrev 2011-02-28 18:31:
>>
>>
>>> -    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.

Yes, but it makes it harder to figure out what actual functional changes 
were made to the code when browsing the git history or doing git blame. 
I'll simply point to what the website has to say:

"NOTE: If you had to put if(){ .. } over a large (> 5 lines) chunk of 
code, then either do NOT change the indentation of the inner part within 
(do not move it to the right)! or do so in a separate commit "
http://ffmpeg.org/developer.html#SEC5

>>       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)

Just put the indentation into a separate patch, and have just adding and 
removing the ifs in the first patch. Shouldn't take long to separate 
them. Then whoever maintains that file should be able to OK it.

/Tomas