[FFmpeg-devel] [PATCH 18/23] lavc/movtextenc: add alpha tag handling

[John Stebbins]

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 at 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.
> > 
> > 
> >