[FFmpeg-devel] [PATCH v3 2/2] fftools/ffmpeg: add support for per frame rotation and flip

Jun Li junli1026 at gmail.com
Thu May 16 12:06:33 EEST 2019


On Thu, May 16, 2019 at 1:21 AM Nicolas George <george at nsup.org> wrote:

> Jun Li (12019-05-16):
> > Fix #6945
> > Current implementaion for autorotate works fine for stream
> > level rotataion but no support for frame level operation
> > and frame flip. This patch is for adding flip support and
> > per frame operations.
>
> Can you explain why you decided to do that in the command-line tools
> rather than in a filter?
>

Sure.
This patch is checking the frame's orientation status, and apply input
filter if necessary.
   frame 1 -> check orientation -> get 2  -> need flip --> goto filter
   frame 2 -> check orientation -> get 1 -> do nothing
   frame 3 -> check orientation -> get 7 -> need flip -> goto filter

The following is my understanding of using a filter to achieve, please
correct me if I am wrong.
   frame 1 -> goto filter -> check orientation -> get 2 -> flip
   frame 2 -> goto filter -> check orientation -> get 1 -> do nothing
   frame 3 -> goto filter -> check orientation -> get 7 -> flip

This means the filter will always active no mater what type of content it
is (because we cannot get orientation until decode the frame).
The above is my understanding, correct me if I am wrong.

> ---
> >  fftools/cmdutils.c      |  9 ++---
> >  fftools/cmdutils.h      |  2 +-
> >  fftools/ffmpeg.c        | 21 ++++++++++-
> >  fftools/ffmpeg.h        |  2 +
> >  fftools/ffmpeg_filter.c | 81 ++++++++++++++++++++++++++++++++++++++---
> >  fftools/ffplay.c        | 28 +++++++++++---
> >  6 files changed, 126 insertions(+), 17 deletions(-)
> >
> > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> > index 9cfbc45c2b..b8bb904419 100644
> > --- a/fftools/cmdutils.c
> > +++ b/fftools/cmdutils.c
> > @@ -2172,13 +2172,12 @@ void *grow_array(void *array, int elem_size, int
> *size, int new_size)
> >      return array;
> >  }
> >
> > -double get_rotation(AVStream *st)
> > +double get_rotation_hflip(int32_t display_matrix[9], int* hflip)
> >  {
> > -    uint8_t* displaymatrix = av_stream_get_side_data(st,
> > -
>  AV_PKT_DATA_DISPLAYMATRIX, NULL);
> >      double theta = 0;
> > -    if (displaymatrix)
> > -        theta = -av_display_rotation_get((int32_t*) displaymatrix);
> > +
> > +    if (display_matrix)
> > +        theta = -av_display_rotation_hflip_get(display_matrix, hflip);
> >
> >      theta -= 360*floor(theta/360 + 0.9/360);
> >
> > diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h
> > index 6e2e0a2acb..dce44ed6e1 100644
> > --- a/fftools/cmdutils.h
> > +++ b/fftools/cmdutils.h
> > @@ -643,6 +643,6 @@ void *grow_array(void *array, int elem_size, int
> *size, int new_size);
> >      char name[128];\
> >      av_get_channel_layout_string(name, sizeof(name), 0, ch_layout);
> >
> > -double get_rotation(AVStream *st);
> > +double get_rotation_hflip(int32_t display_matrix[9], int* hflip);
> >
> >  #endif /* FFTOOLS_CMDUTILS_H */
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 01f04103cf..9ea1aaa930 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2126,6 +2126,24 @@ static int
> ifilter_has_all_input_formats(FilterGraph *fg)
> >      return 1;
> >  }
> >
>
> > +static int ifilter_display_matrix_need_from_frame(InputFilter *ifilter,
> AVFrame* frame)
>
> The name of this function suggests it is just a test, while it seems to
> do more.
>

I see your point and agree with it. Either split the function or rename it,
I prefer rename but still cannot think of a good name.
Let me re-think about it and will address maybe next iteration. I would
appreciate it if you could share some advice. :)

> +{
> > +    int32_t* stream_matrix =
> (int32_t*)av_stream_get_side_data(ifilter->ist->st,
> > +
> AV_PKT_DATA_DISPLAYMATRIX, NULL);
> > +    // if we already have display matrix from stream, use it instead of
> extracting
> > +    // from frame.
> > +    if (stream_matrix) {
> > +        memcpy(ifilter->display_matrix, stream_matrix, 4 * 9);
> > +        return 0;
> > +    }
> > +
> > +    // cleanup the display matrix, may be from last frame
> > +    memset(ifilter->display_matrix, 0, 4 * 9);
> > +    av_display_rotation_set(ifilter->display_matrix, 0);
> > +
> > +    return !!av_dict_get(frame->metadata, "Orientation", NULL, 0);
> > +}
> > +
> >  static int ifilter_send_frame(InputFilter *ifilter, AVFrame *frame)
> >  {
> >      FilterGraph *fg = ifilter->graph;
> > @@ -2141,7 +2159,8 @@ static int ifilter_send_frame(InputFilter
> *ifilter, AVFrame *frame)
> >                         ifilter->channel_layout != frame->channel_layout;
> >          break;
> >      case AVMEDIA_TYPE_VIDEO:
> > -        need_reinit |= ifilter->width  != frame->width ||
> > +        need_reinit |= ifilter_display_matrix_need_from_frame(ifilter,
> frame) ||
> > +                       ifilter->width  != frame->width ||
> >                         ifilter->height != frame->height;
> >          break;
> >      }
> > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> > index eb1eaf6363..8331720663 100644
> > --- a/fftools/ffmpeg.h
> > +++ b/fftools/ffmpeg.h
> > @@ -251,6 +251,8 @@ typedef struct InputFilter {
> >      int channels;
> >      uint64_t channel_layout;
> >
> > +    int32_t display_matrix[9];
> > +
> >      AVBufferRef *hw_frames_ctx;
> >
> >      int eof;
> > diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
> > index 72838de1e2..1894f30561 100644
> > --- a/fftools/ffmpeg_filter.c
> > +++ b/fftools/ffmpeg_filter.c
> > @@ -328,6 +328,7 @@ static void init_input_filter(FilterGraph *fg,
> AVFilterInOut *in)
> >      fg->inputs[fg->nb_inputs - 1]->format = -1;
> >      fg->inputs[fg->nb_inputs - 1]->type = ist->st->codecpar->codec_type;
> >      fg->inputs[fg->nb_inputs - 1]->name = describe_filter_link(fg, in,
> 1);
> > +    av_display_rotation_set(fg->inputs[fg->nb_inputs -
> 1]->display_matrix, 0);
> >
> >      fg->inputs[fg->nb_inputs - 1]->frame_queue = av_fifo_alloc(8 *
> sizeof(AVFrame*));
> >      if (!fg->inputs[fg->nb_inputs - 1]->frame_queue)
> > @@ -807,22 +808,43 @@ static int
> configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
> >      last_filter = ifilter->filter;
> >
> >      if (ist->autorotate) {
>
> > -        double theta = get_rotation(ist->st);
> > +        int hflip = 0;
> > +        double theta = get_rotation_hflip(ifilter->display_matrix,
> &hflip);
> >
> > -        if (fabs(theta - 90) < 1.0) {
> > +        if (fabs(theta) < 1.0) {
> > +            if (hflip)
> > +                ret = insert_filter(&last_filter, &pad_idx, "hflip",
> NULL);
> > +        } else if (fabs(theta - 90) < 1.0) {
> >              ret = insert_filter(&last_filter, &pad_idx, "transpose",
> "clock");
> > -        } else if (fabs(theta - 180) < 1.0) {
> > -            ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
> >              if (ret < 0)
> >                  return ret;
> > -            ret = insert_filter(&last_filter, &pad_idx, "vflip", NULL);
> > +            if (hflip)
> > +                ret = insert_filter(&last_filter, &pad_idx, "hflip",
> NULL);
> > +        } else if (fabs(theta - 180) < 1.0) {
> > +            if (hflip) { // rotate 180 and hflip equals vflip
> > +                ret = insert_filter(&last_filter, &pad_idx, "vflip",
> NULL);
> > +            } else {
> > +                ret = insert_filter(&last_filter, &pad_idx, "hflip",
> NULL);
> > +                if (ret < 0)
> > +                    return ret;
> > +                ret = insert_filter(&last_filter, &pad_idx, "vflip",
> NULL);
> > +            }
> >          } else if (fabs(theta - 270) < 1.0) {
> >              ret = insert_filter(&last_filter, &pad_idx, "transpose",
> "cclock");
> > +            if (ret < 0)
> > +                return ret;
> > +            if (hflip)
> > +                ret = insert_filter(&last_filter, &pad_idx, "hflip",
> NULL);
> >          } else if (fabs(theta) > 1.0) {
> >              char rotate_buf[64];
> >              snprintf(rotate_buf, sizeof(rotate_buf), "%f*PI/180",
> theta);
> >              ret = insert_filter(&last_filter, &pad_idx, "rotate",
> rotate_buf);
> > +            if (ret < 0)
> > +                return ret;
> > +            if (hflip)
> > +                ret = insert_filter(&last_filter, &pad_idx, "hflip",
> NULL);
>
> All this code seems quite redundant with the part in ffplay.

True, I see the code was like that before this. But is the merge work
should be included in this task or address in different one ?


> >          }
> > +
> >          if (ret < 0)
> >              return ret;
> >      }
> > @@ -1182,6 +1204,53 @@ fail:
> >      return ret;
> >  }
> >
> > +static void set_display_matrix_from_frame(const AVFrame *frame, int32_t
> m[9])
> > +{
> > +    AVDictionaryEntry *entry = NULL;
> > +    int orientation = 0;
> > +
> > +    // read exif orientation data
> > +    entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);
> > +    if (entry) {
> > +        orientation = atoi(entry->value);
> > +        memset(m, 0, 4 * 9);
> > +        switch (orientation)
> > +        {
> > +            case 1: // horizontal (normal)
> > +                av_display_rotation_set(m, 0.0);
> > +                break;
> > +            case 2: // mirror horizontal
> > +                av_display_rotation_set(m, 0.0);
> > +                av_display_matrix_flip(m, 1, 0);
> > +                break;
> > +            case 3: // rotate 180
> > +                av_display_rotation_set(m, 180.0);
> > +                break;
> > +            case 4: // mirror vertical
> > +                av_display_rotation_set(m, 0.0);
> > +                av_display_matrix_flip(m, 0, 1);
> > +                break;
> > +            case 5: // mirror horizontal and rotate 270 CW
> > +                av_display_rotation_set(m, 270.0);
> > +                av_display_matrix_flip(m, 0, 1);
> > +                break;
> > +            case 6: // rotate 90 CW
> > +                av_display_rotation_set(m, 90.0);
> > +                break;
> > +            case 7: // mirror horizontal and rotate 90 CW
> > +                av_display_rotation_set(m, 90.0);
> > +                av_display_matrix_flip(m, 0, 1);
> > +                break;
> > +            case 8: // rotate 270 CW
> > +                av_display_rotation_set(m, 270.0);
> > +                break;
> > +            default:
> > +                av_display_rotation_set(m, 0.0);
> > +                break;
> > +        }
> > +    }
> > +}
> > +
> >  int ifilter_parameters_from_frame(InputFilter *ifilter, const AVFrame
> *frame)
> >  {
> >      av_buffer_unref(&ifilter->hw_frames_ctx);
> > @@ -1202,6 +1271,8 @@ int ifilter_parameters_from_frame(InputFilter
> *ifilter, const AVFrame *frame)
> >              return AVERROR(ENOMEM);
> >      }
> >
> > +    set_display_matrix_from_frame(frame, ifilter->display_matrix);
> > +
> >      return 0;
> >  }
> >
> > diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> > index 8f050e16e6..717a9a830c 100644
> > --- a/fftools/ffplay.c
> > +++ b/fftools/ffplay.c
> > @@ -1914,19 +1914,37 @@ static int configure_video_filters(AVFilterGraph
> *graph, VideoState *is, const c
> >  } while (0)
> >
> >      if (autorotate) {
>
> > -        double theta  = get_rotation(is->video_st);
> > -
> > -        if (fabs(theta - 90) < 1.0) {
> > +        int hflip = 0;
> > +        double theta = 0.0;
> > +        int32_t* display_matrix =
> (int32_t*)av_stream_get_side_data(is->video_st,
> > +
> AV_PKT_DATA_DISPLAYMATRIX, NULL);
> > +        if (display_matrix)
> > +             theta  = get_rotation_hflip(display_matrix, &hflip);
> > +
> > +        if (fabs(theta) < 1.0) {
> > +            if (hflip)
> > +                INSERT_FILT("hflip", NULL);
> > +        } else if (fabs(theta - 90) < 1.0) {
> >              INSERT_FILT("transpose", "clock");
> > +            if (hflip)
> > +                INSERT_FILT("hflip", NULL);
> >          } else if (fabs(theta - 180) < 1.0) {
> > -            INSERT_FILT("hflip", NULL);
> > -            INSERT_FILT("vflip", NULL);
> > +            if (hflip) { // rotate 180 and hflip equals vflip
> > +                INSERT_FILT("vflip", NULL);
> > +            } else {
> > +                INSERT_FILT("hflip", NULL);
> > +                INSERT_FILT("vflip", NULL);
> > +            }
> >          } else if (fabs(theta - 270) < 1.0) {
> >              INSERT_FILT("transpose", "cclock");
> > +            if (hflip)
> > +                INSERT_FILT("hflip", NULL);
> >          } else if (fabs(theta) > 1.0) {
> >              char rotate_buf[64];
> >              snprintf(rotate_buf, sizeof(rotate_buf), "%f*PI/180",
> theta);
> >              INSERT_FILT("rotate", rotate_buf);
> > +            if (hflip)
> > +                INSERT_FILT("hflip", NULL);
>
> I see ffmpeg has set_display_matrix_from_frame() but ffplay does not. I
> do not see how this will work with ffplay. Can you explain?
>

I verified the VLC, they do the flip during playing, exactly as you
suggested.
The reason I didn't do that is, to keep the code change small to address
the ticket only.
Surely will address it if you think it's necessary.


> >          }
> >      }
> >
>
> Regards,
>
> --
>   Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".


More information about the ffmpeg-devel mailing list