[FFmpeg-devel,v2,2/2] fftools/ffmpeg: Add stream metadata from first frame's metadata

Submitted by Jun Li on May 6, 2019, 3:41 a.m.

Details

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

Commit Message

Jun Li May 6, 2019, 3:41 a.m.
Fix #6945
Exif extension has 'Orientaion' field for image flip and rotation.
This change is to add the first frame's exif into stream so that
autorotation would use the info to adjust the frames.
---
 fftools/ffmpeg.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Nicolas George May 6, 2019, 8:51 a.m.
Jun Li (12019-05-05):
> Fix #6945
> Exif extension has 'Orientaion' field for image flip and rotation.
> This change is to add the first frame's exif into stream so that
> autorotation would use the info to adjust the frames.

What happens if not all frames have the same orientation?

Regards,
Jun Li May 6, 2019, 9:51 p.m.
On Mon, May 6, 2019 at 1:52 AM Nicolas George <george@nsup.org> wrote:

> Jun Li (12019-05-05):
> > Fix #6945
> > Exif extension has 'Orientaion' field for image flip and rotation.
> > This change is to add the first frame's exif into stream so that
> > autorotation would use the info to adjust the frames.
>
> What happens if not all frames have the same orientation?


Thanks Nicolas for review !
I agree with it that the patch can't solve the case that "not all frames
have the same orientation".
The only way I can think of (correct me if I am wrong) is a filter that
dynamic do tranformation per frame.

From a technical perspective, this kind of media is absolutely possible.
For example motion jpeg consists of jpg with different orientations.
But I assume it is rare with my limited knowledge. What do you think?

Best Regards,
Jun




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".
Carl Eugen Hoyos May 6, 2019, 10:04 p.m.
Am Mo., 6. Mai 2019 um 05:41 Uhr schrieb Jun Li <junli1026@gmail.com>:
> +    uint8_t* sd = NULL;

Wouldn't it simplify the patch if this were a pointer to int32_t?
Or is there a aliasing violation that can't be avoided?

> +    if (entry) {
> +        orientation = atoi(entry->value);
> +        sd = av_stream_new_side_data(ist->st, AV_PKT_DATA_DISPLAYMATRIX,
> +                                                sizeof(int32_t) * 9);

Most of the time, sizeof(type) makes little sense, if you cannot use
sizeof(variable),
just put "4" there.

Carl Eugen
Jun Li May 6, 2019, 10:44 p.m.
On Mon, May 6, 2019 at 3:04 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Am Mo., 6. Mai 2019 um 05:41 Uhr schrieb Jun Li <junli1026@gmail.com>:
> > +    uint8_t* sd = NULL;
>
> Wouldn't it simplify the patch if this were a pointer to int32_t?
> Or is there a aliasing violation that can't be avoided?
>

Thanks Carl for review. Will address in next iteration.



> > +    if (entry) {
> > +        orientation = atoi(entry->value);
> > +        sd = av_stream_new_side_data(ist->st, AV_PKT_DATA_DISPLAYMATRIX,
> > +                                                sizeof(int32_t) * 9);
>
> Most of the time, sizeof(type) makes little sense, if you cannot use
> sizeof(variable),
> just put "4" there.
>

Thanks Carl for review. Will address in next iteration.



> Carl Eugen
> _______________________________________________
> 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".
Michael Niedermayer May 7, 2019, 9:06 p.m.
On Sun, May 05, 2019 at 08:41:28PM -0700, Jun Li wrote:
> Fix #6945
> Exif extension has 'Orientaion' field for image flip and rotation.
> This change is to add the first frame's exif into stream so that
> autorotation would use the info to adjust the frames.
> ---
>  fftools/ffmpeg.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..cea85c601e 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2341,6 +2341,56 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output,

this produces warnings like:
fftools/ffmpeg.c: In function ‘set_metadata_from_1stframe’:
fftools/ffmpeg.c:2362:17: warning: passing argument 1 of ‘av_display_matrix_flip’ from incompatible pointer type [enabled by default]

[...]
Nicolas George May 9, 2019, 8:52 a.m.
Jun Li (12019-05-06):
> I agree with it that the patch can't solve the case that "not all frames
> have the same orientation".
> The only way I can think of (correct me if I am wrong) is a filter that
> dynamic do tranformation per frame.
> 
> From a technical perspective, this kind of media is absolutely possible.
> For example motion jpeg consists of jpg with different orientations.
> But I assume it is rare with my limited knowledge. What do you think?

I think they can be quite common: imagine somebody trying to make a
slideshow from vacation photos.

A filter that applies the transform on each frame based on the metadata
would be my first idea too. We already have the filter: the one that
does the transform right now. It needs to be adapter to change the
transform on a per-frame basis. That should be easy.

But it is a bit harder than that: lavfi does not support real
reconfiguration of the stream properties. Therefore, this filter must be
added to the small list of "temporary" exceptions in the framework. And
the user will need to insert scale/pad/crop to normalize the output.
That needs to be documented.

Regards,
Jun Li May 9, 2019, 9:20 a.m.
On Thu, May 9, 2019 at 1:52 AM Nicolas George <george@nsup.org> wrote:

> Jun Li (12019-05-06):
> > I agree with it that the patch can't solve the case that "not all frames
> > have the same orientation".
> > The only way I can think of (correct me if I am wrong) is a filter that
> > dynamic do tranformation per frame.
> >
> > From a technical perspective, this kind of media is absolutely possible.
> > For example motion jpeg consists of jpg with different orientations.
> > But I assume it is rare with my limited knowledge. What do you think?
>
> I think they can be quite common: imagine somebody trying to make a
> slideshow from vacation photos.
>
> A filter that applies the transform on each frame based on the metadata
> would be my first idea too. We already have the filter: the one that
> does the transform right now. It needs to be adapter to change the
> transform on a per-frame basis. That should be easy.
>
> But it is a bit harder than that: lavfi does not support real
> reconfiguration of the stream properties. Therefore, this filter must be
> added to the small list of "temporary" exceptions in the framework. And
> the user will need to insert scale/pad/crop to normalize the output.
> That needs to be documented.
>
> 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".


Hi Nicolas,
Thanks for the input, I will take a look at the transform filter.

The image's exif ("orientation") info is kept inside of frame metadata,
maybe the filter can
directly read from frame, instead of re-config the stream level metadata ?
How about that ?

Best Regards,
Jun
Nicolas George May 9, 2019, 9:29 a.m.
Jun Li (12019-05-09):
> The image's exif ("orientation") info is kept inside of frame metadata,
> maybe the filter can directly read from frame, instead of re-config
> the stream level metadata ?

That is exactly what I was suggesting.

Regards,
Jun Li May 9, 2019, 9:50 a.m.
On Thu, May 9, 2019 at 2:29 AM Nicolas George <george@nsup.org> wrote:

> Jun Li (12019-05-09):
> > The image's exif ("orientation") info is kept inside of frame metadata,
> > maybe the filter can directly read from frame, instead of re-config
> > the stream level metadata ?
>
> That is exactly what I was suggesting.
>
> 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".


Hi Nicolas,
I see  transpose filter and vflip/hvlip filter, but could not find
"transform".
are you suggesting creating a new one achieve both transpose and flip, or
modify transpose to support flip ?

-Jun
Nicolas George May 9, 2019, 9:54 a.m.
Jun Li (12019-05-09):
> I see  transpose filter and vflip/hvlip filter, but could not find
> "transform".

I do not think I suggested a filter with that name exists.

> are you suggesting creating a new one achieve both transpose and flip, or
> modify transpose to support flip ?

You will also need rotate, I think.

Merging the filters is the best option, I think. I realize it requires
more work, but I do not think we can achieve something that works well
otherwise.

Regards,
Paul B Mahol May 9, 2019, 10:57 a.m.
On 5/9/19, Nicolas George <george@nsup.org> wrote:
> Jun Li (12019-05-09):
>> I see  transpose filter and vflip/hvlip filter, but could not find
>> "transform".
>
> I do not think I suggested a filter with that name exists.
>
>> are you suggesting creating a new one achieve both transpose and flip, or
>> modify transpose to support flip ?
>
> You will also need rotate, I think.
>
> Merging the filters is the best option, I think. I realize it requires
> more work, but I do not think we can achieve something that works well
> otherwise.

Merging of what filters?

Your reasoning is illogical.
Nicolas George May 9, 2019, 2:31 p.m.
Paul B Mahol (12019-05-09):
> Merging of what filters?

transpose and rotate when it is an integer quarter of turns. But
apparently they are already merged.

> Your reasoning is illogical.

Your reasoning is absent.
Paul B Mahol May 9, 2019, 2:50 p.m.
On 5/9/19, Nicolas George <george@nsup.org> wrote:
> Paul B Mahol (12019-05-09):
>> Merging of what filters?
>
> transpose and rotate when it is an integer quarter of turns. But
> apparently they are already merged.
>
>> Your reasoning is illogical.
>
> Your reasoning is absent.
>

Give mathematical proof.
Jun Li May 11, 2019, 3:11 a.m.
On Thu, May 9, 2019 at 1:52 AM Nicolas George <george@nsup.org> wrote:

> Jun Li (12019-05-06):
> > I agree with it that the patch can't solve the case that "not all frames
> > have the same orientation".
> > The only way I can think of (correct me if I am wrong) is a filter that
> > dynamic do tranformation per frame.
> >
> > From a technical perspective, this kind of media is absolutely possible.
> > For example motion jpeg consists of jpg with different orientations.
> > But I assume it is rare with my limited knowledge. What do you think?
>
> I think they can be quite common: imagine somebody trying to make a
> slideshow from vacation photos.
>
> A filter that applies the transform on each frame based on the metadata
> would be my first idea too. We already have the filter: the one that
> does the transform right now. It needs to be adapter to change the
> transform on a per-frame basis. That should be easy.
>
> But it is a bit harder than that: lavfi does not support real
> reconfiguration of the stream properties. Therefore, this filter must be
> added to the small list of "temporary" exceptions in the framework. And
> the user will need to insert scale/pad/crop to normalize the output.
> That needs to be documented.
>

Hi Nicolas,
I think now I understand what you mean"lavfi does not support real
reconfiguration of the stream properties. "
Even we have a perfect filter for this case, we still have to dynamic
turn on/off the filter per frame's metadata.

Then I just wonder maybe we just need a function to rotate/flip the frame
data,
rather than using filter ? Do you mind sharing some advice ?

Thanks
-Jun

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 11, 2019, 2:32 p.m.
Jun Li (12019-05-10):
> I think now I understand what you mean"lavfi does not support real
> reconfiguration of the stream properties. "
> Even we have a perfect filter for this case, we still have to dynamic
> turn on/off the filter per frame's metadata.

No, that is not what I meant: there is nothing to turn on or off if the
filters itself examines the frame metadata.

The problem is that if a filter does that, it can switch from outputting
1920×1080 to 1080×1920 when the orientation changes. Most filters and
the framework do not support that.

> Then I just wonder maybe we just need a function to rotate/flip the frame
> data, rather than using filter ? Do you mind sharing some advice ?

That would not change anything, since it was for the wrong problem.

Regards,

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 01f04103cf..cea85c601e 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2341,6 +2341,56 @@  static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output,
     return err < 0 ? err : ret;
 }
 
+static void set_metadata_from_1stframe(InputStream* ist, AVFrame* frame) 
+{
+    // read exif Orientation data
+    AVDictionaryEntry *entry = av_dict_get(frame->metadata, "Orientation", NULL, 0);
+    int orientation = 0;
+    uint8_t* sd = NULL;
+    if (entry) {
+        orientation = atoi(entry->value);
+        sd = av_stream_new_side_data(ist->st, AV_PKT_DATA_DISPLAYMATRIX, 
+                                                sizeof(int32_t) * 9);
+        memset(sd, 0, sizeof(int32_t) * 9);
+        switch (orientation)
+        {
+            case 1: // horizontal (normal)
+                av_display_rotation_set((int32_t*)sd, 0.0);
+                break;
+            case 2: // mirror horizontal
+                av_display_rotation_set((int32_t*)sd, 0.0);
+                av_display_matrix_flip(sd, 1, 0);
+                break;
+            case 3: // rotate 180
+                av_display_rotation_set((int32_t*)sd, 180.0);
+                break;
+            case 4: // mirror vertical
+                av_display_rotation_set((int32_t*)sd, 0.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 5: // mirror horizontal and rotate 270 CW
+                av_display_rotation_set((int32_t*)sd, 270.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 6: // rotate 90 CW
+                av_display_rotation_set((int32_t*)sd, 90.0);
+                break;
+            case 7: // mirror horizontal and rotate 90 CW
+                av_display_rotation_set((int32_t*)sd, 90.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 8: // rotate 270 CW
+                av_display_rotation_set((int32_t*)sd, 270.0);
+                break;
+            default:
+                av_display_rotation_set((int32_t*)sd, 0.0);
+                av_log(ist->dec_ctx, AV_LOG_WARNING,    
+                    "Exif orientation data out of range: %i. Set to default value: horizontal(normal).", orientation);
+                break;
+        }
+    }
+}
+
 static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_t *duration_pts, int eof,
                         int *decode_failed)
 {
@@ -2423,6 +2473,8 @@  static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_
         decoded_frame->top_field_first = ist->top_field_first;
 
     ist->frames_decoded++;
+    if (ist->frames_decoded == 1)
+        set_metadata_from_1stframe(ist, decoded_frame);
 
     if (ist->hwaccel_retrieve_data && decoded_frame->format == ist->hwaccel_pix_fmt) {
         err = ist->hwaccel_retrieve_data(ist->dec_ctx, decoded_frame);