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

Submitted by Jun Li on May 8, 2019, 6:24 a.m.

Details

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

Commit Message

Jun Li May 8, 2019, 6:24 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

Comments

Gyan May 8, 2019, 6:40 a.m.
On 08-05-2019 11:54 AM, 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.

Suggest commit msg should be

"

'Orientation' field from EXIF tags in first decoded frame is extracted
  as stream side data so that ffmpeg can apply auto-rotation.

"


> ---
>   fftools/ffmpeg.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 01f04103cf..98ccaf0732 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2341,6 +2341,58 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output,
>       return err < 0 ? err : ret;
>   }
>   
> +static int 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;
> +    int32_t* sd = NULL;
> +    if (entry) {
> +        orientation = atoi(entry->value);
> +        sd = (int32_t*)av_stream_new_side_data(ist->st, AV_PKT_DATA_DISPLAYMATRIX, 4 * 9);
> +        if (!sd)
> +            return AVERROR(ENOMEM);
> +        memset(sd, 0, 4 * 9);
> +        switch (orientation)
> +        {
> +            case 1: // horizontal (normal)
> +                av_display_rotation_set(sd, 0.0);
> +                break;
> +            case 2: // mirror horizontal
> +                av_display_rotation_set(sd, 0.0);
> +                av_display_matrix_flip(sd, 1, 0);
> +                break;
> +            case 3: // rotate 180
> +                av_display_rotation_set(sd, 180.0);
> +                break;
> +            case 4: // mirror vertical
> +                av_display_rotation_set(sd, 0.0);
> +                av_display_matrix_flip(sd, 0, 1);
> +                break;
> +            case 5: // mirror horizontal and rotate 270 CW
> +                av_display_rotation_set(sd, 270.0);
> +                av_display_matrix_flip(sd, 0, 1);
> +                break;
> +            case 6: // rotate 90 CW
> +                av_display_rotation_set(sd, 90.0);
> +                break;
> +            case 7: // mirror horizontal and rotate 90 CW
> +                av_display_rotation_set(sd, 90.0);
> +                av_display_matrix_flip(sd, 0, 1);
> +                break;
> +            case 8: // rotate 270 CW
> +                av_display_rotation_set(sd, 270.0);
> +                break;
> +            default:
> +                av_display_rotation_set(sd, 0.0);
> +                av_log(ist->dec_ctx, AV_LOG_WARNING,
> +                    "Exif orientation data out of range: %i. Set to default value: horizontal(normal).\n", orientation);
> +                break;
> +        }
> +    }
> +    return 0;
> +}
> +
>   static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_t *duration_pts, int eof,
>                           int *decode_failed)
>   {
> @@ -2423,7 +2475,10 @@ 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 &&
> +        ((err = set_metadata_from_1stframe(ist, decoded_frame)) < 0))
> +        goto fail;
> +
>       if (ist->hwaccel_retrieve_data && decoded_frame->format == ist->hwaccel_pix_fmt) {
>           err = ist->hwaccel_retrieve_data(ist->dec_ctx, decoded_frame);
>           if (err < 0)

Also, is there a chance that there may be multiple sources for 
orientation data available for a given stream? If yes, what's the 
interaction? It looks like you append a new SD element.

Gyan
Jun Li May 8, 2019, 6:55 a.m.
On Tue, May 7, 2019 at 11:40 PM Gyan <ffmpeg@gyani.pro> wrote:

>
>
> On 08-05-2019 11:54 AM, 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.
>
> Suggest commit msg should be
>
> "
>
> 'Orientation' field from EXIF tags in first decoded frame is extracted
>   as stream side data so that ffmpeg can apply auto-rotation.
>
> "
>

Sure, will address that in next iteration.


>
> > ---
> >   fftools/ffmpeg.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index 01f04103cf..98ccaf0732 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -2341,6 +2341,58 @@ static int decode_audio(InputStream *ist,
> AVPacket *pkt, int *got_output,
> >       return err < 0 ? err : ret;
> >   }
> >
> > +static int 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;
> > +    int32_t* sd = NULL;
> > +    if (entry) {
> > +        orientation = atoi(entry->value);
> > +        sd = (int32_t*)av_stream_new_side_data(ist->st,
> AV_PKT_DATA_DISPLAYMATRIX, 4 * 9);
> > +        if (!sd)
> > +            return AVERROR(ENOMEM);
> > +        memset(sd, 0, 4 * 9);
> > +        switch (orientation)
> > +        {
> > +            case 1: // horizontal (normal)
> > +                av_display_rotation_set(sd, 0.0);
> > +                break;
> > +            case 2: // mirror horizontal
> > +                av_display_rotation_set(sd, 0.0);
> > +                av_display_matrix_flip(sd, 1, 0);
> > +                break;
> > +            case 3: // rotate 180
> > +                av_display_rotation_set(sd, 180.0);
> > +                break;
> > +            case 4: // mirror vertical
> > +                av_display_rotation_set(sd, 0.0);
> > +                av_display_matrix_flip(sd, 0, 1);
> > +                break;
> > +            case 5: // mirror horizontal and rotate 270 CW
> > +                av_display_rotation_set(sd, 270.0);
> > +                av_display_matrix_flip(sd, 0, 1);
> > +                break;
> > +            case 6: // rotate 90 CW
> > +                av_display_rotation_set(sd, 90.0);
> > +                break;
> > +            case 7: // mirror horizontal and rotate 90 CW
> > +                av_display_rotation_set(sd, 90.0);
> > +                av_display_matrix_flip(sd, 0, 1);
> > +                break;
> > +            case 8: // rotate 270 CW
> > +                av_display_rotation_set(sd, 270.0);
> > +                break;
> > +            default:
> > +                av_display_rotation_set(sd, 0.0);
> > +                av_log(ist->dec_ctx, AV_LOG_WARNING,
> > +                    "Exif orientation data out of range: %i. Set to
> default value: horizontal(normal).\n", orientation);
> > +                break;
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> >   static int decode_video(InputStream *ist, AVPacket *pkt, int
> *got_output, int64_t *duration_pts, int eof,
> >                           int *decode_failed)
> >   {
> > @@ -2423,7 +2475,10 @@ 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 &&
> > +        ((err = set_metadata_from_1stframe(ist, decoded_frame)) < 0))
> > +        goto fail;
> > +
> >       if (ist->hwaccel_retrieve_data && decoded_frame->format ==
> ist->hwaccel_pix_fmt) {
> >           err = ist->hwaccel_retrieve_data(ist->dec_ctx, decoded_frame);
> >           if (err < 0)
>
> Also, is there a chance that there may be multiple sources for
> orientation data available for a given stream? If yes, what's the
> interaction? It looks like you append a new SD element.
>

Thanks Gyan for review !
Nicolas George asked the same question before. :)

Yes, this patch can't handle the case every frame has its own orientation.
From a technical perspective, it is absolutely possible, for example a
motion jpeg stream with different orientation value
on every frame. I think an ideal solution for this case is a filter doing
transformation per orientation every frame.

But based on my limited experience, I think this kind of content is rare.
What do you think ?
Maybe someone in the community met this kind of content before ?

Gyan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Gyan May 8, 2019, 7:08 a.m.
On 08-05-2019 12:25 PM, Jun Li wrote:
> On Tue, May 7, 2019 at 11:40 PM Gyan <ffmpeg@gyani.pro> wrote:
>
>
>> Also, is there a chance that there may be multiple sources for
>> orientation data available for a given stream? If yes, what's the
>> interaction? It looks like you append a new SD element.
>>
> Thanks Gyan for review !
> Nicolas George asked the same question before. :)
>
> Yes, this patch can't handle the case every frame has its own orientation.
>  From a technical perspective, it is absolutely possible, for example a
> motion jpeg stream with different orientation value
> on every frame. I think an ideal solution for this case is a filter doing
> transformation per orientation every frame.

I'm not referring to dynamic per-frame orientation.

I'm wondering about a scenario where the container has orientation 
metadata and so does the packet payload (which can only be accessed by 
the decoder). Is there any possibility of that happening? What if they 
are different?

Gyan
Jun Li May 8, 2019, 7:32 a.m.
On Wed, May 8, 2019 at 12:09 AM Gyan <ffmpeg@gyani.pro> wrote:

>
>
> On 08-05-2019 12:25 PM, Jun Li wrote:
> > On Tue, May 7, 2019 at 11:40 PM Gyan <ffmpeg@gyani.pro> wrote:
> >
> >
> >> Also, is there a chance that there may be multiple sources for
> >> orientation data available for a given stream? If yes, what's the
> >> interaction? It looks like you append a new SD element.
> >>
> > Thanks Gyan for review !
> > Nicolas George asked the same question before. :)
> >
> > Yes, this patch can't handle the case every frame has its own
> orientation.
> >  From a technical perspective, it is absolutely possible, for example a
> > motion jpeg stream with different orientation value
> > on every frame. I think an ideal solution for this case is a filter doing
> > transformation per orientation every frame.
>
> I'm not referring to dynamic per-frame orientation.
>
> I'm wondering about a scenario where the container has orientation
> metadata and so does the packet payload (which can only be accessed by
> the decoder). Is there any possibility of that happening? What if they
> are different?
>
> Gyan
> _______________________________________________
> 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".


Good point !
I do not know the answer. Let's keep it open for a while to see anyone in
the community has the answer.
If no one reply the question, I am going to add some logic to skip the sd
whenever the side_data is already set by by demuxer from container level.
Thanks Gyan !

Best Regards,
Jun
Jun Li May 9, 2019, 6:15 a.m.
On Wed, May 8, 2019 at 12:32 AM Jun Li <junli1026@gmail.com> wrote:

>
>
> On Wed, May 8, 2019 at 12:09 AM Gyan <ffmpeg@gyani.pro> wrote:
>
>>
>>
>> On 08-05-2019 12:25 PM, Jun Li wrote:
>> > On Tue, May 7, 2019 at 11:40 PM Gyan <ffmpeg@gyani.pro> wrote:
>> >
>> >
>> >> Also, is there a chance that there may be multiple sources for
>> >> orientation data available for a given stream? If yes, what's the
>> >> interaction? It looks like you append a new SD element.
>> >>
>> > Thanks Gyan for review !
>> > Nicolas George asked the same question before. :)
>> >
>> > Yes, this patch can't handle the case every frame has its own
>> orientation.
>> >  From a technical perspective, it is absolutely possible, for example a
>> > motion jpeg stream with different orientation value
>> > on every frame. I think an ideal solution for this case is a filter
>> doing
>> > transformation per orientation every frame.
>>
>> I'm not referring to dynamic per-frame orientation.
>>
>> I'm wondering about a scenario where the container has orientation
>> metadata and so does the packet payload (which can only be accessed by
>> the decoder). Is there any possibility of that happening? What if they
>> are different?
>>
>> Gyan
>> _______________________________________________
>> 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".
>
>
> Good point !
> I do not know the answer. Let's keep it open for a while to see anyone in
> the community has the answer.
> If no one reply the question, I am going to add some logic to skip the sd
> whenever the side_data is already set by by demuxer from container level.
> Thanks Gyan !
>
> Best Regards,
> Jun
>

New version is sent out to address the issue.

Patch hide | download patch | download mbox

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 01f04103cf..98ccaf0732 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2341,6 +2341,58 @@  static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output,
     return err < 0 ? err : ret;
 }
 
+static int 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;
+    int32_t* sd = NULL;
+    if (entry) {
+        orientation = atoi(entry->value);
+        sd = (int32_t*)av_stream_new_side_data(ist->st, AV_PKT_DATA_DISPLAYMATRIX, 4 * 9);
+        if (!sd)
+            return AVERROR(ENOMEM);
+        memset(sd, 0, 4 * 9);
+        switch (orientation)
+        {
+            case 1: // horizontal (normal)
+                av_display_rotation_set(sd, 0.0);
+                break;
+            case 2: // mirror horizontal
+                av_display_rotation_set(sd, 0.0);
+                av_display_matrix_flip(sd, 1, 0);
+                break;
+            case 3: // rotate 180
+                av_display_rotation_set(sd, 180.0);
+                break;
+            case 4: // mirror vertical
+                av_display_rotation_set(sd, 0.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 5: // mirror horizontal and rotate 270 CW
+                av_display_rotation_set(sd, 270.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 6: // rotate 90 CW
+                av_display_rotation_set(sd, 90.0);
+                break;
+            case 7: // mirror horizontal and rotate 90 CW
+                av_display_rotation_set(sd, 90.0);
+                av_display_matrix_flip(sd, 0, 1);
+                break;
+            case 8: // rotate 270 CW
+                av_display_rotation_set(sd, 270.0);
+                break;
+            default:
+                av_display_rotation_set(sd, 0.0);
+                av_log(ist->dec_ctx, AV_LOG_WARNING,    
+                    "Exif orientation data out of range: %i. Set to default value: horizontal(normal).\n", orientation);
+                break;
+        }
+    }
+    return 0;
+}
+
 static int decode_video(InputStream *ist, AVPacket *pkt, int *got_output, int64_t *duration_pts, int eof,
                         int *decode_failed)
 {
@@ -2423,7 +2475,10 @@  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 &&
+        ((err = set_metadata_from_1stframe(ist, decoded_frame)) < 0))
+        goto fail;
+    
     if (ist->hwaccel_retrieve_data && decoded_frame->format == ist->hwaccel_pix_fmt) {
         err = ist->hwaccel_retrieve_data(ist->dec_ctx, decoded_frame);
         if (err < 0)