[FFmpeg-devel] [PATCH 1/1] Updated version of patch 1840 (ticket 6021)

[Moritz Barsnick]

On Mon, Dec 19, 2016 at 03:05:18 +0000, Erik Bråthen Solem wrote:

> Subject: [FFmpeg-devel] [PATCH 1/1] Updated version of patch 1840 (ticket 6021)

You need to give a proper commit message. Do have a look at other
commits in the repo.

The first line of the message (which is shown in this subject, the way
you are submitting your patch here) would be something like:

lavc/movtextdec: fix incorrect offset calculation for UTF-8 characters

(i.e. describe what it does [to what], not what it is)
and then an empty line and then the full description, which then also
has an extra line at the end such as:

Fixes trac #6021.

> Between testing and patch generation a character was deleted by mistake, which
> broke the patch. This updated version fixes this.

The way you are submitting your patch here, this text becomes part of
the commit message, but it doesn't belong there.

>      while (text < text_end) {
> -        if (m->box_flags & STYL_BOX) {
> -            for (i = 0; i < m->style_entries; i++) {
> -                if (m->s[i]->style_flag && text_pos == m->s[i]->style_end) {
> -                    av_bprintf(buf, "{\\r}");
> +        if ((*text & 0xC0) != 0x80) { // Boxes never split multibyte characters
> +            if (m->box_flags & STYL_BOX) {
> +                for (i = 0; i < m->style_entries; i++) {
> +                    if (m->s[i]->style_flag &&
> +                        text_pos_chars == m->s[i]->style_end)
> +                    {
> +                        av_bprintf(buf, "{\\r}");
> +                    }
>                  }

You are doing two to three things at the same time in this part of the
patch:
- wrapping existing code inside an if() clause or shifting its
  layering: fine
- re-indenting the code that has now moved into a new block:
  this is hard to review (i.e. to see that the block has remained
  unchanged) and it is usually requested that you do the re-indentation
  in a follow-up patch.
- re-formatting the code: even if useful, falls into the previous
  category
- re-formatting to non-ffmpeg style: that's a no-go.

(Let me guess that your editor is doing the reformatting for you.
Usually, that's nifty. ;-))

I believe the patch would be *much* easier to read and much shorter if
you only did the very first step in the initial patch.

> -        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize, m->d.color,
> -                                m->d.back_color, m->d.bold, m->d.italic,
> -                                m->d.underline, ASS_DEFAULT_BORDERSTYLE,
> -                                m->d.alignment);
> +        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize,
> +                                m->d.color, m->d.back_color, m->d.bold,
> +                                m->d.italic, m->d.underline,
> +                                ASS_DEFAULT_BORDERSTYLE, m->d.alignment);

And as far as I can tell, this is *only* reformatting. This also
belongs into a separate patch (if at all useful).

> -                    if (m->tracksize + m->size_var + box_types[i].base_size > avpkt->size)
> +                    if (m->tracksize + m->size_var +
> +                        box_types[i].base_size > avpkt->size)

Same here.

Cheers,
Moritz