On Thu, May 16, 2019 at 2:06 AM Jun Li <junli1...@gmail.com> wrote:
>
>
> On Thu, May 16, 2019 at 1:21 AM Nicolas George <geo...@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-requ...@ffmpeg.org with subject "unsubscribe".
>
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
