Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[Neil Birkbeck]

On Fri, Apr 15, 2016 at 4:06 AM wm4  wrote:

> On Wed, 6 Apr 2016 17:09:21 -0600
> Nic Wolfe  wrote:
>
> > Thanks for elaborating wm4. Out of curiosity when you say "generic"
> > and "normal" metadata what do you mean? Are you talking about using
> > generic key names or is there another way to store metadata?
> >
> > I have attached a patch which adds a new AVPacketSideDataType and uses
> > that type to store crop values in the AVStream's side data if they're
> > found during Matroska demuxing. I have included a comment in avcodec.h
> > with some of the info Dave shared above but I'm not sure if that is
> > sufficient to call this "well defined".
> >
> > I'm aware that this is a complex topic and this patch is certainly not
> > comprehensive but hopefully it is a step in the right direction.
>
> I think this is much better. The problem are still the semantics. I
> tried to discuss it in Libav, but nothing conclusive has emerged.
>
> Maybe you could send this patch to libav-devel for discussion? (I feel
> a bit sorry for playing this ping pong with a first time contributor.
> If anyone has a better idea how to continue this, please make a
> suggestion.)
>
> By the way, it's not a good idea to send base64 encoded patches. I
> don't think most mail clients can expand them automatically. It
> makes review harder. Just attach them normally. Also, try to avoid
> top-posting.
>

Reviving this thread, as it seems to have the most context on the topic of
PixelCrop in MKV.

I was also investigating adding support for PixelCrop* fields (
https://matroska.org/technical/specs/index.html) for a particular case of
cropping additional padding off the bottom of the image (e.g.,
PixelWidth=1920, PixelHeight=1088, PixelCropBottom=8,
PixelCrop{Top,Right,Left}=0, DisplayWidth=1920, DisplayHeight=1080) which
is required due to a hardware constraint (and codec bitrstream doesn't
support the crop, unlike say h264/hevc). For my use-case, propagating the
MKV fields through to the AVFrame::crop* and silently applying the crop
would be ideal behavior, but I realize that may not generalize to others.

It seems there is still a bunch of complexity around the various edge cases
and interaction with metadata in the container/codec (e.g., which one takes
precedent?).  And the PixelCrop fields also come up in (
https://trac.ffmpeg.org/ticket/7437), where it seems the fields might be a
natural destination for contents of mov/mp4 "clap" atom (
https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html#//apple_ref/doc/uid/TP4939-CH205-125850).
This seems partially relevant, as perhaps a common SideData element can
contain the "clap" and  "PixelCrop*" (if possible)--making the interface
more consistent for applications and transmuxing cases

Is an incremental way forward to:
-Support application level support by extract the PixelCrop fields and
exporting as AVStream side data (perhaps works for 'clap' atom too). [say
by revisiting the AVPacketSideData patch in this thread]
-Support transmuxing use case: add support to muxers, mapping between
relevant containers, where/if applicable.
-Support auto-apply decode case. I guess this is close to what autorotate
does. Need to be sure to strip strip metadata so it doesn't get muxed into
output.


Any alternatives, or objections, to at least exporting these fields as
stream side data as a first step?


> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
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".

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[wm4]

On Wed, 6 Apr 2016 17:09:21 -0600
Nic Wolfe  wrote:

> Thanks for elaborating wm4. Out of curiosity when you say "generic"
> and "normal" metadata what do you mean? Are you talking about using
> generic key names or is there another way to store metadata?
> 
> I have attached a patch which adds a new AVPacketSideDataType and uses
> that type to store crop values in the AVStream's side data if they're
> found during Matroska demuxing. I have included a comment in avcodec.h
> with some of the info Dave shared above but I'm not sure if that is
> sufficient to call this "well defined".
> 
> I'm aware that this is a complex topic and this patch is certainly not
> comprehensive but hopefully it is a step in the right direction.

I think this is much better. The problem are still the semantics. I
tried to discuss it in Libav, but nothing conclusive has emerged.

Maybe you could send this patch to libav-devel for discussion? (I feel
a bit sorry for playing this ping pong with a first time contributor.
If anyone has a better idea how to continue this, please make a
suggestion.)

By the way, it's not a good idea to send base64 encoded patches. I
don't think most mail clients can expand them automatically. It
makes review harder. Just attach them normally. Also, try to avoid
top-posting.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[Nic Wolfe]

Thanks for elaborating wm4. Out of curiosity when you say "generic"
and "normal" metadata what do you mean? Are you talking about using
generic key names or is there another way to store metadata?

I have attached a patch which adds a new AVPacketSideDataType and uses
that type to store crop values in the AVStream's side data if they're
found during Matroska demuxing. I have included a comment in avcodec.h
with some of the info Dave shared above but I'm not sure if that is
sufficient to call this "well defined".

I'm aware that this is a complex topic and this patch is certainly not
comprehensive but hopefully it is a step in the right direction.

Thanks,

Nic

On Wed, Mar 30, 2016 at 2:38 AM, Steve Lhomme  wrote:
> On Tue, Mar 29, 2016 at 2:46 PM, Dave Rice  wrote:
>> Hi,
>>
>>> On Mar 29, 2016, at 4:23 AM, wm4  wrote:
>>>
>>> On Sat, 26 Mar 2016 16:56:55 -0600
>>> Nic Wolfe  wrote:
>>>
 The Matroska spec defines PixelCropTop, PixelCropBottom, PixelCropLeft,
 and PixelCropRight elements: 
 https://www.matroska.org/technical/specs/index.html

 This commit adds support for demuxing these values so that
 applications using libav*
 are able to use them when playing the stream. They're added to the 
 AVStream's
 metadata if they are set to something non-zero.


 My official patch is base64 encoded and attached but I will also
 include the diff below for (hopefully) convenience.

>>>
>>> To elaborate on why this change is bad (in its current state):
>>>
>>> - It's not clearly defined what the pixelcrop fields mean. Do they
>>>  operate before or after aspect rasto is applied? Do they affect
>>>  aspect ratio calculation? What if aspect ratio or video size change
>>>  later? Does it get applied after h264 bitstream cropping, does it
>>>  override it? AFAIK these issues were also discussed on the cellar
>>>  mailing list, but I didn't follow it.
>>
>> It was discussed on CELLAR but not patched yet. At the moment, the best 
>> clarification is from Steve Lhomme in this post 
>> https://mailarchive.ietf.org/arch/search/?email_list=cellar=display+area+question:
>>  "Yes, the cropping happens on the pixels, the display size are just how to 
>> display those remaining pixels.” So the pixelcrop is applied before the 
>> aspect ratio.
>
> Given in the past pixel cropping was done in parallel to the codec one
> (ie redefining the ones already implied by the codec like 1088 MPEG2)
> and there is a need to allow different cropping that the internal
> codec one, we might need some extra flags to cover both cases. Since
> they could be both needed, we might need a set of extra UserPixelCrop.
> I think mkvmerge (at least) copies the codec crop into the current
> PixelCrop, which would become CodecPixelCrop in the documentation.
>
>> AFAIK there is no determination as to how h264 bitstream cropping and 
>> Matroska cropping should be prioritized (cc’ing CELLAR on this).
>>
>> I think the PixelCrop documentation also needs to consider handling in video 
>> stereo modes.
>>
>>> - There should generally be a concept at least in libavformat's API how
>>>  to handle cropping. For example, it could be some sort of well-defined
>>>  AVStream side data. (Personally I'd be a fan of adding it to AVFrame
>>>  too. There's no way around if it should work for hw decoding.)
>>> - Worst of all: it's exported as generic metadata. This means that:
>>>  - API users could start interpreting the same metadata fields for
>>>other formats than Matroska too
>>>  - Transcoders like ffmpeg CLI will copy the crop metadata to other
>>>containers (as normal metadata).
>>>  - Non-Matroska files might be created that contain the "made
>>>up" libavformat Matroska demuxer metadata, and the creator of the
>>>file expects that programs respect it.
>>>  (Something like this almost happened with the "old" libavformat
>>>  rotation metadata, which is also exported as normal metadata.)
>>>
>>> While just adding a "hack" to export metadata for essentially 1 API
>>> user might be acceptable if adding "proper" API is too hard for now, at
>>> least the last point needs to be fixed.
>>
>> I also support a demuxer option to allow pixelcrop to be ignored.
>> Best Regards,
>> Dave Rice
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


0001-lavf-matroskadec-Demux-the-PixelCrop-values.patch.base64
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[Steve Lhomme]

On Tue, Mar 29, 2016 at 2:46 PM, Dave Rice  wrote:
> Hi,
>
>> On Mar 29, 2016, at 4:23 AM, wm4  wrote:
>>
>> On Sat, 26 Mar 2016 16:56:55 -0600
>> Nic Wolfe  wrote:
>>
>>> The Matroska spec defines PixelCropTop, PixelCropBottom, PixelCropLeft,
>>> and PixelCropRight elements: 
>>> https://www.matroska.org/technical/specs/index.html
>>>
>>> This commit adds support for demuxing these values so that
>>> applications using libav*
>>> are able to use them when playing the stream. They're added to the 
>>> AVStream's
>>> metadata if they are set to something non-zero.
>>>
>>>
>>> My official patch is base64 encoded and attached but I will also
>>> include the diff below for (hopefully) convenience.
>>>
>>
>> To elaborate on why this change is bad (in its current state):
>>
>> - It's not clearly defined what the pixelcrop fields mean. Do they
>>  operate before or after aspect rasto is applied? Do they affect
>>  aspect ratio calculation? What if aspect ratio or video size change
>>  later? Does it get applied after h264 bitstream cropping, does it
>>  override it? AFAIK these issues were also discussed on the cellar
>>  mailing list, but I didn't follow it.
>
> It was discussed on CELLAR but not patched yet. At the moment, the best 
> clarification is from Steve Lhomme in this post 
> https://mailarchive.ietf.org/arch/search/?email_list=cellar=display+area+question:
>  "Yes, the cropping happens on the pixels, the display size are just how to 
> display those remaining pixels.” So the pixelcrop is applied before the 
> aspect ratio.

Given in the past pixel cropping was done in parallel to the codec one
(ie redefining the ones already implied by the codec like 1088 MPEG2)
and there is a need to allow different cropping that the internal
codec one, we might need some extra flags to cover both cases. Since
they could be both needed, we might need a set of extra UserPixelCrop.
I think mkvmerge (at least) copies the codec crop into the current
PixelCrop, which would become CodecPixelCrop in the documentation.

> AFAIK there is no determination as to how h264 bitstream cropping and 
> Matroska cropping should be prioritized (cc’ing CELLAR on this).
>
> I think the PixelCrop documentation also needs to consider handling in video 
> stereo modes.
>
>> - There should generally be a concept at least in libavformat's API how
>>  to handle cropping. For example, it could be some sort of well-defined
>>  AVStream side data. (Personally I'd be a fan of adding it to AVFrame
>>  too. There's no way around if it should work for hw decoding.)
>> - Worst of all: it's exported as generic metadata. This means that:
>>  - API users could start interpreting the same metadata fields for
>>other formats than Matroska too
>>  - Transcoders like ffmpeg CLI will copy the crop metadata to other
>>containers (as normal metadata).
>>  - Non-Matroska files might be created that contain the "made
>>up" libavformat Matroska demuxer metadata, and the creator of the
>>file expects that programs respect it.
>>  (Something like this almost happened with the "old" libavformat
>>  rotation metadata, which is also exported as normal metadata.)
>>
>> While just adding a "hack" to export metadata for essentially 1 API
>> user might be acceptable if adding "proper" API is too hard for now, at
>> least the last point needs to be fixed.
>
> I also support a demuxer option to allow pixelcrop to be ignored.
> Best Regards,
> Dave Rice
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[Dave Rice]

Hi,

> On Mar 29, 2016, at 4:23 AM, wm4  wrote:
> 
> On Sat, 26 Mar 2016 16:56:55 -0600
> Nic Wolfe  wrote:
> 
>> The Matroska spec defines PixelCropTop, PixelCropBottom, PixelCropLeft,
>> and PixelCropRight elements: 
>> https://www.matroska.org/technical/specs/index.html
>> 
>> This commit adds support for demuxing these values so that
>> applications using libav*
>> are able to use them when playing the stream. They're added to the AVStream's
>> metadata if they are set to something non-zero.
>> 
>> 
>> My official patch is base64 encoded and attached but I will also
>> include the diff below for (hopefully) convenience.
>> 
> 
> To elaborate on why this change is bad (in its current state):
> 
> - It's not clearly defined what the pixelcrop fields mean. Do they
>  operate before or after aspect rasto is applied? Do they affect
>  aspect ratio calculation? What if aspect ratio or video size change
>  later? Does it get applied after h264 bitstream cropping, does it
>  override it? AFAIK these issues were also discussed on the cellar
>  mailing list, but I didn't follow it.

It was discussed on CELLAR but not patched yet. At the moment, the best 
clarification is from Steve Lhomme in this post 
https://mailarchive.ietf.org/arch/search/?email_list=cellar=display+area+question:
 "Yes, the cropping happens on the pixels, the display size are just how to 
display those remaining pixels.” So the pixelcrop is applied before the aspect 
ratio.

AFAIK there is no determination as to how h264 bitstream cropping and Matroska 
cropping should be prioritized (cc’ing CELLAR on this).

I think the PixelCrop documentation also needs to consider handling in video 
stereo modes.

> - There should generally be a concept at least in libavformat's API how
>  to handle cropping. For example, it could be some sort of well-defined
>  AVStream side data. (Personally I'd be a fan of adding it to AVFrame
>  too. There's no way around if it should work for hw decoding.)
> - Worst of all: it's exported as generic metadata. This means that:
>  - API users could start interpreting the same metadata fields for
>other formats than Matroska too
>  - Transcoders like ffmpeg CLI will copy the crop metadata to other
>containers (as normal metadata).
>  - Non-Matroska files might be created that contain the "made
>up" libavformat Matroska demuxer metadata, and the creator of the
>file expects that programs respect it.
>  (Something like this almost happened with the "old" libavformat
>  rotation metadata, which is also exported as normal metadata.)
> 
> While just adding a "hack" to export metadata for essentially 1 API
> user might be acceptable if adding "proper" API is too hard for now, at
> least the last point needs to be fixed.

I also support a demuxer option to allow pixelcrop to be ignored.
Best Regards,
Dave Rice
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[wm4]

On Sat, 26 Mar 2016 16:56:55 -0600
Nic Wolfe  wrote:

> The Matroska spec defines PixelCropTop, PixelCropBottom, PixelCropLeft,
> and PixelCropRight elements: 
> https://www.matroska.org/technical/specs/index.html
> 
> This commit adds support for demuxing these values so that
> applications using libav*
> are able to use them when playing the stream. They're added to the AVStream's
> metadata if they are set to something non-zero.
> 
> 
> My official patch is base64 encoded and attached but I will also
> include the diff below for (hopefully) convenience.
> 

To elaborate on why this change is bad (in its current state):

- It's not clearly defined what the pixelcrop fields mean. Do they
  operate before or after aspect rasto is applied? Do they affect
  aspect ratio calculation? What if aspect ratio or video size change
  later? Does it get applied after h264 bitstream cropping, does it
  override it? AFAIK these issues were also discussed on the cellar
  mailing list, but I didn't follow it.
- There should generally be a concept at least in libavformat's API how
  to handle cropping. For example, it could be some sort of well-defined
  AVStream side data. (Personally I'd be a fan of adding it to AVFrame
  too. There's no way around if it should work for hw decoding.)
- Worst of all: it's exported as generic metadata. This means that:
  - API users could start interpreting the same metadata fields for
other formats than Matroska too
  - Transcoders like ffmpeg CLI will copy the crop metadata to other
containers (as normal metadata).
  - Non-Matroska files might be created that contain the "made
up" libavformat Matroska demuxer metadata, and the creator of the
file expects that programs respect it.
  (Something like this almost happened with the "old" libavformat
  rotation metadata, which is also exported as normal metadata.)

While just adding a "hack" to export metadata for essentially 1 API
user might be acceptable if adding "proper" API is too hard for now, at
least the last point needs to be fixed.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/matroskadec: Demux the PixelCrop* values

[wm4]

On Sat, 26 Mar 2016 16:56:55 -0600
Nic Wolfe  wrote:

> The Matroska spec defines PixelCropTop, PixelCropBottom, PixelCropLeft,
> and PixelCropRight elements: 
> https://www.matroska.org/technical/specs/index.html
> 
> This commit adds support for demuxing these values so that
> applications using libav*
> are able to use them when playing the stream. They're added to the AVStream's
> metadata if they are set to something non-zero.

That's a bad way to do it and you know it.

> 
> 
> My official patch is base64 encoded and attached but I will also

Don't do this, I doubt most mail clients provide a way to easily view
this.

> include the diff below for (hopefully) convenience.
> 
> Thanks,
> 
> Nic
> 
> 
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index d788232..72537df 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -139,6 +139,10 @@ typedef struct MatroskaTrackVideo {
>  EbmlBin color_space;
>  uint64_t stereo_mode;
>  uint64_t alpha_mode;
> +uint64_t crop_bottom;
> +uint64_t crop_top;
> +uint64_t crop_left;
> +uint64_t crop_right;
>  } MatroskaTrackVideo;
> 
>  typedef struct MatroskaTrackAudio {
> @@ -364,10 +368,10 @@ static const EbmlSyntax matroska_track_video[] = {
>  { MATROSKA_ID_VIDEOPIXELHEIGHT,EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, pixel_height) },
>  { MATROSKA_ID_VIDEOCOLORSPACE, EBML_BIN,   0,
> offsetof(MatroskaTrackVideo, color_space) },
>  { MATROSKA_ID_VIDEOALPHAMODE,  EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, alpha_mode) },
> -{ MATROSKA_ID_VIDEOPIXELCROPB, EBML_NONE },
> -{ MATROSKA_ID_VIDEOPIXELCROPT, EBML_NONE },
> -{ MATROSKA_ID_VIDEOPIXELCROPL, EBML_NONE },
> -{ MATROSKA_ID_VIDEOPIXELCROPR, EBML_NONE },
> +{ MATROSKA_ID_VIDEOPIXELCROPB, EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, crop_bottom) },
> +{ MATROSKA_ID_VIDEOPIXELCROPT, EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, crop_top) },
> +{ MATROSKA_ID_VIDEOPIXELCROPL, EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, crop_left) },
> +{ MATROSKA_ID_VIDEOPIXELCROPR, EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, crop_right) },
>  { MATROSKA_ID_VIDEODISPLAYUNIT,EBML_NONE },
>  { MATROSKA_ID_VIDEOFLAGINTERLACED, EBML_NONE },
>  { MATROSKA_ID_VIDEOSTEREOMODE, EBML_UINT,  0,
> offsetof(MatroskaTrackVideo, stereo_mode), { .u =
> MATROSKA_VIDEO_STEREOMODE_TYPE_NB } },
> @@ -2152,6 +2156,16 @@ static int matroska_parse_tracks(AVFormatContext *s)
>  if (track->video.stereo_mode && track->video.stereo_mode
> < MATROSKA_VIDEO_STEREOMODE_TYPE_NB)
>  av_dict_set(>metadata, "stereo_mode",
> ff_matroska_video_stereo_mode[track->video.stereo_mode], 0);
> 
> +/* export the matroska crop settings as metadata */
> +if (track->video.crop_bottom != 0)
> +av_dict_set_int(>metadata, "crop_bottom",
> track->video.crop_bottom, 0);
> +if (track->video.crop_top != 0)
> +av_dict_set_int(>metadata, "crop_top",
> track->video.crop_top, 0);
> +if (track->video.crop_left != 0)
> +av_dict_set_int(>metadata, "crop_left",
> track->video.crop_left, 0);
> +if (track->video.crop_right != 0)
> +av_dict_set_int(>metadata, "crop_right",
> track->video.crop_right, 0);
> +
>  /* export alpha mode flag as metadata tag  */
>  if (track->video.alpha_mode)
>  av_dict_set(>metadata, "alpha_mode", "1", 0);

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel