[PATCH] lavf/matroskadec: Demux the PixelCrop* values
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. 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(&st->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(&st->metadata, "crop_bottom", track->video.crop_bottom, 0); + if (track->video.crop_top != 0) + av_dict_set_int(&st->metadata, "crop_top", track->video.crop_top, 0); + if (track->video.crop_left != 0) + av_dict_set_int(&st->metadata, "crop_left", track->video.crop_left, 0); + if (track->video.crop_right != 0) + av_dict_set_int(&st->metadata, "crop_right", track->video.crop_right, 0); + /* export alpha mode flag as metadata tag */ if (track->video.alpha_mode) av_dict_set(&st->metadata, "alpha_mode", "1", 0);
On Sat, 26 Mar 2016 16:56:55 -0600 Nic Wolfe <nic@wolfeden.ca> 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(&st->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(&st->metadata, "crop_bottom", track->video.crop_bottom, 0); + if (track->video.crop_top != 0) + av_dict_set_int(&st->metadata, "crop_top", track->video.crop_top, 0); + if (track->video.crop_left != 0) + av_dict_set_int(&st->metadata, "crop_left", track->video.crop_left, 0); + if (track->video.crop_right != 0) + av_dict_set_int(&st->metadata, "crop_right", track->video.crop_right, 0); + /* export alpha mode flag as metadata tag */ if (track->video.alpha_mode) av_dict_set(&st->metadata, "alpha_mode", "1", 0);
On Sat, 26 Mar 2016 16:56:55 -0600 Nic Wolfe <nic@wolfeden.ca> 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.
Hi,
On Mar 29, 2016, at 4:23 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Sat, 26 Mar 2016 16:56:55 -0600 Nic Wolfe <nic@wolfeden.ca> 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&q=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
On Tue, Mar 29, 2016 at 2:46 PM, Dave Rice <dave@dericed.com> wrote:
Hi,
On Mar 29, 2016, at 4:23 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Sat, 26 Mar 2016 16:56:55 -0600 Nic Wolfe <nic@wolfeden.ca> 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&q=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
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 <robux4@videolabs.io> wrote:
On Tue, Mar 29, 2016 at 2:46 PM, Dave Rice <dave@dericed.com> wrote:
Hi,
On Mar 29, 2016, at 4:23 AM, wm4 <nfxjfg@googlemail.com> wrote:
On Sat, 26 Mar 2016 16:56:55 -0600 Nic Wolfe <nic@wolfeden.ca> 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&q=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
On Wed, 6 Apr 2016 17:09:21 -0600 Nic Wolfe <nic@wolfeden.ca> 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.
On Fri, Apr 15, 2016 at 4:06 AM wm4 <nfxjfg@googlemail.com> wrote:
On Wed, 6 Apr 2016 17:09:21 -0600 Nic Wolfe <nic@wolfeden.ca> 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/QTF...). 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. <https://trac.ffmpeg.org/ticket/7437> 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
participants (5)
-
Dave Rice -
Neil Birkbeck -
Nic Wolfe -
Steve Lhomme -
wm4