| Message ID | 20190516071222.30876-2-junli1026@gmail.com |
|---|---|
| State | New |
| Headers | show |
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,
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".
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". > >
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,
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 >
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
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".
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".
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".
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
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); } }