| Message ID | 20200406175218.1299994-19-jstebbins@jetheaddev.com |
|---|---|
| State | Accepted |
| Commit | 2e79843e3750582239f9aa26b1dbe1429a18dc71 |
| Headers | show |
| Series | [FFmpeg-devel,01/23] lavc/movtextdec: fix ass header colors | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | warning | Failed to apply patch |
On Mon, 6 Apr 2020 11:52:13 -0600 John Stebbins <jstebbins@jetheaddev.com> wrote: > --- > libavcodec/movtextenc.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c > index 090536b887..e82393dde7 100644 > --- a/libavcodec/movtextenc.c > +++ b/libavcodec/movtextenc.c > @@ -351,6 +351,26 @@ static void mov_text_color_cb(void *priv, > unsigned int color, unsigned int color */ > } > > +static void mov_text_alpha_set(MovTextContext *s, uint8_t alpha) > +{ > + if (!s->style_attributes_temp || > + (s->style_attributes_temp->style_color & 0xff) == alpha) { > + // color hasn't changed > + return; > + } > + if (mov_text_style_start(s)) > + s->style_attributes_temp->style_color = > + (s->style_attributes_temp->style_color & 0xffffff00) > | alpha; +} > + > +static void mov_text_alpha_cb(void *priv, int alpha, int alpha_id) > +{ > + MovTextContext *s = priv; > + > + if (alpha_id == 1) // primary alpha changes > + mov_text_alpha_set(s, 255 - alpha); > +} Worth a comment that secondary alpha can't be preserved? > + > static void mov_text_end_cb(void *priv) > { > // End of text, close any open style record > @@ -360,7 +380,7 @@ static void mov_text_end_cb(void *priv) > static void mov_text_dialog(MovTextContext *s, ASSDialog *dialog) > { > ASSStyle * style = ff_ass_style_get(s->ass_ctx, dialog->style); > - uint8_t style_flags; > + uint8_t style_flags, alpha; > uint32_t color; > > if (style) { > @@ -370,6 +390,8 @@ static void mov_text_dialog(MovTextContext *s, > ASSDialog *dialog) mov_text_style_set(s, style_flags); > color = BGR_TO_RGB(style->primary_color & 0xffffff) << 8; > mov_text_color_set(s, color); > + alpha = 255 - ((uint32_t)style->primary_color >> 24); > + mov_text_alpha_set(s, alpha); > } > } > > @@ -416,6 +438,7 @@ static const ASSCodesCallbacks mov_text_callbacks > = { .new_line = mov_text_new_line_cb, > .style = mov_text_style_cb, > .color = mov_text_color_cb, > + .alpha = mov_text_alpha_cb, > .end = mov_text_end_cb, > }; > Otherwise LGTM. --phil
On Wed, 2020-04-08 at 11:37 -0700, Philip Langdale wrote: > On Mon, 6 Apr 2020 11:52:13 -0600 > John Stebbins <jstebbins@jetheaddev.com> wrote: > > > --- > > libavcodec/movtextenc.c | 25 ++++++++++++++++++++++++- > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c > > index 090536b887..e82393dde7 100644 > > --- a/libavcodec/movtextenc.c > > +++ b/libavcodec/movtextenc.c > > @@ -351,6 +351,26 @@ static void mov_text_color_cb(void *priv, > > unsigned int color, unsigned int color */ > > } > > > > +static void mov_text_alpha_set(MovTextContext *s, uint8_t alpha) > > +{ > > + if (!s->style_attributes_temp || > > + (s->style_attributes_temp->style_color & 0xff) == alpha) { > > + // color hasn't changed > > + return; > > + } > > + if (mov_text_style_start(s)) > > + s->style_attributes_temp->style_color = > > + (s->style_attributes_temp->style_color & > > 0xffffff00) > > > alpha; +} > > + > > +static void mov_text_alpha_cb(void *priv, int alpha, int alpha_id) > > +{ > > + MovTextContext *s = priv; > > + > > + if (alpha_id == 1) // primary alpha changes > > + mov_text_alpha_set(s, 255 - alpha); > > +} > > Worth a comment that secondary alpha can't be preserved? Sure, will do. > > > + > > static void mov_text_end_cb(void *priv) > > { > > // End of text, close any open style record > > @@ -360,7 +380,7 @@ static void mov_text_end_cb(void *priv) > > static void mov_text_dialog(MovTextContext *s, ASSDialog *dialog) > > { > > ASSStyle * style = ff_ass_style_get(s->ass_ctx, dialog- > > >style); > > - uint8_t style_flags; > > + uint8_t style_flags, alpha; > > uint32_t color; > > > > if (style) { > > @@ -370,6 +390,8 @@ static void mov_text_dialog(MovTextContext *s, > > ASSDialog *dialog) mov_text_style_set(s, style_flags); > > color = BGR_TO_RGB(style->primary_color & 0xffffff) << 8; > > mov_text_color_set(s, color); > > + alpha = 255 - ((uint32_t)style->primary_color >> 24); > > + mov_text_alpha_set(s, alpha); > > } > > } > > > > @@ -416,6 +438,7 @@ static const ASSCodesCallbacks > > mov_text_callbacks > > = { .new_line = mov_text_new_line_cb, > > .style = mov_text_style_cb, > > .color = mov_text_color_cb, > > + .alpha = mov_text_alpha_cb, > > .end = mov_text_end_cb, > > }; > > > > Otherwise LGTM. > > > > --phil > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, 2020-04-09 at 08:23 -0700, John Stebbins wrote: > On Wed, 2020-04-08 at 11:37 -0700, Philip Langdale wrote: > > On Mon, 6 Apr 2020 11:52:13 -0600 > > John Stebbins <jstebbins@jetheaddev.com> wrote: > > > > > --- > > > libavcodec/movtextenc.c | 25 ++++++++++++++++++++++++- > > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c > > > index 090536b887..e82393dde7 100644 > > > --- a/libavcodec/movtextenc.c > > > +++ b/libavcodec/movtextenc.c > > > @@ -351,6 +351,26 @@ static void mov_text_color_cb(void *priv, > > > unsigned int color, unsigned int color */ > > > } > > > > > > +static void mov_text_alpha_set(MovTextContext *s, uint8_t alpha) > > > +{ > > > + if (!s->style_attributes_temp || > > > + (s->style_attributes_temp->style_color & 0xff) == alpha) > > > { > > > + // color hasn't changed > > > + return; > > > + } > > > + if (mov_text_style_start(s)) > > > + s->style_attributes_temp->style_color = > > > + (s->style_attributes_temp->style_color & > > > 0xffffff00) > > > > alpha; +} > > > + > > > +static void mov_text_alpha_cb(void *priv, int alpha, int > > > alpha_id) > > > +{ > > > + MovTextContext *s = priv; > > > + > > > + if (alpha_id == 1) // primary alpha changes > > > + mov_text_alpha_set(s, 255 - alpha); > > > +} > > > > Worth a comment that secondary alpha can't be preserved? > > Sure, will do. Taking a second look at this, I'm wondering if alpha_id = 2 should be applied to the highlight color that is set in mov_text_color_cb? And if so, should a highlight box be started if only the alpha is changed and not the color? And if so, should the highlight color default to the current primary color when only alpha changes? I'm not familiar with how mov text highlight works and what it looks like in practice. Do you have an opinion? If I add the above, I'll do that as a separate new patch, along with the comment below. There should still be a comment here (and probably for color as well) that outline and background colors can not be modified in mov text. > > > > + > > > static void mov_text_end_cb(void *priv) > > > { > > > // End of text, close any open style record > > > @@ -360,7 +380,7 @@ static void mov_text_end_cb(void *priv) > > > static void mov_text_dialog(MovTextContext *s, ASSDialog > > > *dialog) > > > { > > > ASSStyle * style = ff_ass_style_get(s->ass_ctx, dialog- > > > > style); > > > - uint8_t style_flags; > > > + uint8_t style_flags, alpha; > > > uint32_t color; > > > > > > if (style) { > > > @@ -370,6 +390,8 @@ static void mov_text_dialog(MovTextContext > > > *s, > > > ASSDialog *dialog) mov_text_style_set(s, style_flags); > > > color = BGR_TO_RGB(style->primary_color & 0xffffff) << > > > 8; > > > mov_text_color_set(s, color); > > > + alpha = 255 - ((uint32_t)style->primary_color >> 24); > > > + mov_text_alpha_set(s, alpha); > > > } > > > } > > > > > > @@ -416,6 +438,7 @@ static const ASSCodesCallbacks > > > mov_text_callbacks > > > = { .new_line = mov_text_new_line_cb, > > > .style = mov_text_style_cb, > > > .color = mov_text_color_cb, > > > + .alpha = mov_text_alpha_cb, > > > .end = mov_text_end_cb, > > > }; > > > > > > > Otherwise LGTM. > > > > > >
On Thu, 9 Apr 2020 16:17:21 +0000 John Stebbins <jstebbins@jetheaddev.com> wrote: > > > > > > Worth a comment that secondary alpha can't be preserved? > > > > Sure, will do. > > Taking a second look at this, I'm wondering if alpha_id = 2 should be > applied to the highlight color that is set in mov_text_color_cb? And > if so, should a highlight box be started if only the alpha is changed > and not the color? And if so, should the highlight color default to > the current primary color when only alpha changes? I'm not familiar > with how mov text highlight works and what it looks like in practice. > Do you have an opinion? > > If I add the above, I'll do that as a separate new patch, along with > the comment below. > > There should still be a comment here (and probably for color as well) > that outline and background colors can not be modified in mov text. I'm afraid I don't know either. I've not encountered any truly rich movtext subtitles so I don't know what these things are supposed to look like. It sounds like a reasonable argument to say you can apply an alpha-only highlight, but I couldn't even say if that's in-spec or not. :-) I agree that handling anything like that can be a separate patch. --phil
diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c index 090536b887..e82393dde7 100644 --- a/libavcodec/movtextenc.c +++ b/libavcodec/movtextenc.c @@ -351,6 +351,26 @@ static void mov_text_color_cb(void *priv, unsigned int color, unsigned int color */ } +static void mov_text_alpha_set(MovTextContext *s, uint8_t alpha) +{ + if (!s->style_attributes_temp || + (s->style_attributes_temp->style_color & 0xff) == alpha) { + // color hasn't changed + return; + } + if (mov_text_style_start(s)) + s->style_attributes_temp->style_color = + (s->style_attributes_temp->style_color & 0xffffff00) | alpha; +} + +static void mov_text_alpha_cb(void *priv, int alpha, int alpha_id) +{ + MovTextContext *s = priv; + + if (alpha_id == 1) // primary alpha changes + mov_text_alpha_set(s, 255 - alpha); +} + static void mov_text_end_cb(void *priv) { // End of text, close any open style record @@ -360,7 +380,7 @@ static void mov_text_end_cb(void *priv) static void mov_text_dialog(MovTextContext *s, ASSDialog *dialog) { ASSStyle * style = ff_ass_style_get(s->ass_ctx, dialog->style); - uint8_t style_flags; + uint8_t style_flags, alpha; uint32_t color; if (style) { @@ -370,6 +390,8 @@ static void mov_text_dialog(MovTextContext *s, ASSDialog *dialog) mov_text_style_set(s, style_flags); color = BGR_TO_RGB(style->primary_color & 0xffffff) << 8; mov_text_color_set(s, color); + alpha = 255 - ((uint32_t)style->primary_color >> 24); + mov_text_alpha_set(s, alpha); } } @@ -416,6 +438,7 @@ static const ASSCodesCallbacks mov_text_callbacks = { .new_line = mov_text_new_line_cb, .style = mov_text_style_cb, .color = mov_text_color_cb, + .alpha = mov_text_alpha_cb, .end = mov_text_end_cb, };