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

Submitted by Jun Li on May 16, 2019, 7:12 a.m.

Details

Message ID 20190516071222.30876-2-junli1026@gmail.com
State New
Headers show

Commit Message

Jun Li May 16, 2019, 7:12 a.m.
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.
---
 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(-)

Comments

Nicolas George May 16, 2019, 8:21 a.m.
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?

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

> +{
> +    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.

>          }
> +
>          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?

>          }
>      }
>  

Regards,
Jun Li May 16, 2019, 9:06 a.m.
On Thu, May 16, 2019 at 1:21 AM Nicolas George <george@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jun Li May 16, 2019, 4 p.m.
On Thu, May 16, 2019 at 2:06 AM Jun Li <junli1026@gmail.com> wrote:

>
>
> On Thu, May 16, 2019 at 1:21 AM Nicolas George <george@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.
>

Hi Nicolas,
I rethink the problem, realized that I should keep the code change small
and focus on fixing #6945, while this ffplay issue is not related to #6945.
So I am going to revert the change on ffplay and keep this patch focusing
on ffmpeg only. The ffplay rotation/flip should be addressed in other patch
but not here.

>
>
>> >          }
>> >      }
>> >
>>
>> Regards,
>>
>> --
>>   Nicolas George
>> _______________________________________________
>> 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".
>
>
Nicolas George May 16, 2019, 6:15 p.m.
Jun Li (12019-05-16):
> 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.

Yes, that is exactly what I mean: update the transpose filter so that it
has a mode where it applies the rotation from metadata automatically.
The overhead for a single filter is rather small; we have a few cases
where a filter is inserted to do nothing, just to be a placeholder.

> > 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. :)

This is entirely your choice, depending on how you want to organize your
code. But if you make it the work of the filter, you do not need that
code at all.

> > 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 ?

My opinion is that if a patch makes a duplicated piece of code more
complex, then it should de-duplicate it beforehand.

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

In the end, you decide what you want to do.

Regards,
Paul B Mahol May 16, 2019, 6:56 p.m.
On 5/16/19, Nicolas George <george@nsup.org> wrote:
> Jun Li (12019-05-16):
>> 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.
>
> Yes, that is exactly what I mean: update the transpose filter so that it
> has a mode where it applies the rotation from metadata automatically.
> The overhead for a single filter is rather small; we have a few cases
> where a filter is inserted to do nothing, just to be a placeholder.

Would it modify size of output frame by any chance?

>
>> > 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. :)
>
> This is entirely your choice, depending on how you want to organize your
> code. But if you make it the work of the filter, you do not need that
> code at all.
>
>> > 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 ?
>
> My opinion is that if a patch makes a duplicated piece of code more
> complex, then it should de-duplicate it beforehand.
>
>> 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.
>
> In the end, you decide what you want to do.
>
> Regards,
>
> --
>   Nicolas George
>
Andriy Gelman May 16, 2019, 7:48 p.m.
Hi Jun, 

On Thu, 16. May 00:12, Jun Li wrote:
> 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.
> ---
>  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)
> +{
> +    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);
>          }

Just a thought, if you model your transformation matrix as M = RH, where H is
the horizontal flip and R is the rotation, is it possible to simplify the logic
here to:

//your H transformation
if (hflip)  
  ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);

//the R transformation 
  same code as before.. 

Thanks, 
Andriy
Jun Li May 17, 2019, 1:23 a.m.
On Thu, May 16, 2019 at 12:04 PM Paul B Mahol <onemda@gmail.com> wrote:

> On 5/16/19, Nicolas George <george@nsup.org> wrote:
> > Jun Li (12019-05-16):
> >> 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.
> >
> > Yes, that is exactly what I mean: update the transpose filter so that it
> > has a mode where it applies the rotation from metadata automatically.
> > The overhead for a single filter is rather small; we have a few cases
> > where a filter is inserted to do nothing, just to be a placeholder.
>

Thanks Nicolas for clarifying the overhead of a filter.
That is the only reason why I choose the other path, avoid filter for
non-rotate/flip frames.
I will try to implement it as suggested.


Would it modify size of output frame by any chance?


Hi Paul,
I believe so, like from landscape to portrait when rotation is applied.
The frame might be stretched in this case if we make a video from sequence
of images. Codec "copy" may still work since it should not falls into this
code path.
But correct me if I am wrong. I didnot test it. Thanks.

Best Regards,
-Jun

>
> >> > 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. :)
> >
> > This is entirely your choice, depending on how you want to organize your
> > code. But if you make it the work of the filter, you do not need that
> > code at all.
> >
> >> > 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 ?
> >
> > My opinion is that if a patch makes a duplicated piece of code more
> > complex, then it should de-duplicate it beforehand.
> >
> >> 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.
> >
> > In the end, you decide what you want to do.
> >
> > Regards,
> >
> > --
> >   Nicolas George
> >
> _______________________________________________
> 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".
Jun Li May 17, 2019, 1:28 a.m.
On Thu, May 16, 2019 at 12:54 PM Andriy Gelman <andriy.gelman@gmail.com>
wrote:

> Hi Jun,
>
> On Thu, 16. May 00:12, Jun Li wrote:
> > 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.
> > ---
> >  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)
> > +{
> > +    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);
> >          }
>
> Just a thought, if you model your transformation matrix as M = RH, where H
> is
> the horizontal flip and R is the rotation, is it possible to simplify the
> logic
> here to:
>
> //your H transformation
> if (hflip)
>   ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
>
> //the R transformation
>   same code as before..
>

Sorry Andriy, but I donot quite get it. Do you mean put this code
   //your H transformation
   if (hflip)
       ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
outside of the  if check ?

Best Regards,
Jun

Thanks,
> Andriy
> _______________________________________________
> 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".
Ben Hutchinson May 17, 2019, 7:47 a.m.
Please unsubscribe me from this mailing list. It is filling up my inbox. Or
at least switch me to getting a daily digest from the mailing list. The way
it is set up now, I'm getting an email for every post, and my inbox is
cluttered.

On Thu, May 16, 2019 at 12:21 AM Jun Li <junli1026@gmail.com> wrote:

> 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.
> ---
>  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)
> +{
> +    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);
>          }
> +
>          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);
>          }
>      }
>
> --
> 2.17.1
>
> _______________________________________________
> 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".
Gyan May 17, 2019, 8:02 a.m.
On 17-05-2019 01:17 PM, Ben Hutchinson wrote:
> Please unsubscribe me from this mailing list. It is filling up my inbox. Or
> at least switch me to getting a daily digest from the mailing list. The way
> it is set up now, I'm getting an email for every post, and my inbox is
> cluttered.
...
> _______________________________________________
> 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".

See above.

Gyan

Patch hide | download patch | download mbox

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)
+{
+    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);
         }
+
         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);
         }
     }