| Message ID | 20190506034128.22929-2-junli1026@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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,
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".
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
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".
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] [...]
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,
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
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,
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
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,
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.
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.
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.
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".
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,
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);