[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 <robux4 at videolabs.io> wrote:
> On Tue, Mar 29, 2016 at 2:46 PM, Dave Rice <dave at dericed.com> wrote:
>> Hi,
>>
>>> On Mar 29, 2016, at 4:23 AM, wm4 <nfxjfg at googlemail.com> wrote:
>>>
>>> On Sat, 26 Mar 2016 16:56:55 -0600
>>> Nic Wolfe <nic at 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 at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavf-matroskadec-Demux-the-PixelCrop-values.patch.base64
Type: application/octet-stream
Size: 6853 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160406/e8e8ba90/attachment.obj>