| Message ID | 20200406175218.1299994-2-jstebbins@jetheaddev.com |
|---|---|
| State | Accepted |
| Commit | 47e88adc0dc54250117158dc62e19c1b722f4f01 |
| Headers | show |
| Series | [FFmpeg-devel,01/23] lavc/movtextdec: fix ass header colors | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
John Stebbins (12020-04-06): > A conversion from rgb to bgr is necessary > --- > libavcodec/movtextdec.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c > index c38c5edce6..05becaf64d 100644 > --- a/libavcodec/movtextdec.c > +++ b/libavcodec/movtextdec.c > @@ -48,6 +48,8 @@ > #define TOP_CENTER 8 > #define TOP_RIGHT 9 > > +#define RGB_TO_BGR(c) (((c) & 0xff) << 16 | ((c) & 0xff00) | (((c) >> 16) & 0xff)) > + > typedef struct { > char *font; > int fontsize; > @@ -448,10 +450,11 @@ static int mov_text_init(AVCodecContext *avctx) { > MovTextContext *m = avctx->priv_data; > ret = mov_text_tx3g(avctx, m); > if (ret == 0) { > - 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, > + RGB_TO_BGR(m->d.color), > + RGB_TO_BGR(m->d.back_color), > + m->d.bold, m->d.italic, m->d.underline, > + ASS_DEFAULT_BORDERSTYLE, m->d.alignment); Indentation is off. It was off before, but since the lines are changed it should be ok now. Can't judge on semantic. > } else > return ff_ass_subtitle_header_default(avctx); > } Regards,
On Mon, 6 Apr 2020 11:51:56 -0600 John Stebbins <jstebbins@jetheaddev.com> wrote: > A conversion from rgb to bgr is necessary > --- > libavcodec/movtextdec.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c > index c38c5edce6..05becaf64d 100644 > --- a/libavcodec/movtextdec.c > +++ b/libavcodec/movtextdec.c > @@ -48,6 +48,8 @@ > #define TOP_CENTER 8 > #define TOP_RIGHT 9 > > +#define RGB_TO_BGR(c) (((c) & 0xff) << 16 | ((c) & 0xff00) | (((c) > >> 16) & 0xff)) + > typedef struct { > char *font; > int fontsize; > @@ -448,10 +450,11 @@ static int mov_text_init(AVCodecContext *avctx) > { MovTextContext *m = avctx->priv_data; > ret = mov_text_tx3g(avctx, m); > if (ret == 0) { > - 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, > + RGB_TO_BGR(m->d.color), > + RGB_TO_BGR(m->d.back_color), > + m->d.bold, m->d.italic, m->d.underline, > + ASS_DEFAULT_BORDERSTYLE, m->d.alignment); > } else > return ff_ass_subtitle_header_default(avctx); > } LGTM. --phil
On Mon, 2020-04-06 at 20:03 +0200, Nicolas George wrote: > John Stebbins (12020-04-06): > > A conversion from rgb to bgr is necessary > > --- > > libavcodec/movtextdec.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c > > index c38c5edce6..05becaf64d 100644 > > --- a/libavcodec/movtextdec.c > > +++ b/libavcodec/movtextdec.c > > @@ -48,6 +48,8 @@ > > #define TOP_CENTER 8 > > #define TOP_RIGHT 9 > > > > +#define RGB_TO_BGR(c) (((c) & 0xff) << 16 | ((c) & 0xff00) | (((c) > > >> 16) & 0xff)) > > + > > typedef struct { > > char *font; > > int fontsize; > > @@ -448,10 +450,11 @@ static int mov_text_init(AVCodecContext > > *avctx) { > > MovTextContext *m = avctx->priv_data; > > ret = mov_text_tx3g(avctx, m); > > if (ret == 0) { > > - 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, > > + RGB_TO_BGR(m->d.color), > > + RGB_TO_BGR(m->d.back_color), > > + m->d.bold, m->d.italic, m->d.underline, > > + ASS_DEFAULT_BORDERSTYLE, m->d.alignment); > > Indentation is off. It was off before, but since the lines are > changed > it should be ok now. > > Can't judge on semantic. > > > } else > > return ff_ass_subtitle_header_default(avctx); > > } > > I missed this review earlier. What about the indentation is off? Do you mean the indent of the function parameters does not align after the opening paren? I was keeping the lines aligned without wrapping at 80 characters.
On Thu, 9 Apr 2020 15:09:45 +0000 John Stebbins <jstebbins@jetheaddev.com> wrote: > > > > I missed this review earlier. What about the indentation is off? Do > you mean the indent of the function parameters does not align after > the opening paren? I was keeping the lines aligned without wrapping > at 80 characters. Yes, I believe he's referring to the function parameters and not the overall indent. I would normally align to the paren. I think you can do that and keep under 80 if you put each one on its own line? --phil
On Thu, 2020-04-09 at 09:43 -0700, Philip Langdale wrote: > On Thu, 9 Apr 2020 15:09:45 +0000 > John Stebbins <jstebbins@jetheaddev.com> wrote: > > > > > > > > I missed this review earlier. What about the indentation is > > off? Do > > you mean the indent of the function parameters does not align after > > the opening paren? I was keeping the lines aligned without > > wrapping > > at 80 characters. > > Yes, I believe he's referring to the function parameters and not the > overall indent. I would normally align to the paren. I think you can > do > that and keep under 80 if you put each one on its own line? > > The lines get longer in a subsequent patch (7) and do not fit without wrapping. So I would just have to undo the indent in that patch ;) I could conceivable break lines at expression operators instead of commas, but I think that would make reading more difficult.
diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c index c38c5edce6..05becaf64d 100644 --- a/libavcodec/movtextdec.c +++ b/libavcodec/movtextdec.c @@ -48,6 +48,8 @@ #define TOP_CENTER 8 #define TOP_RIGHT 9 +#define RGB_TO_BGR(c) (((c) & 0xff) << 16 | ((c) & 0xff00) | (((c) >> 16) & 0xff)) + typedef struct { char *font; int fontsize; @@ -448,10 +450,11 @@ static int mov_text_init(AVCodecContext *avctx) { MovTextContext *m = avctx->priv_data; ret = mov_text_tx3g(avctx, m); if (ret == 0) { - 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, + RGB_TO_BGR(m->d.color), + RGB_TO_BGR(m->d.back_color), + m->d.bold, m->d.italic, m->d.underline, + ASS_DEFAULT_BORDERSTYLE, m->d.alignment); } else return ff_ass_subtitle_header_default(avctx); }