[FFmpeg-devel] avfilter: initial macroblock types export and visualization

Submitted by Paul B Mahol on Oct. 27, 2017, 8:03 p.m.

Details

Message ID 20171027200354.31214-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Oct. 27, 2017, 8:03 p.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/mpegvideo.c     |  10 +++++
 libavfilter/vf_codecview.c | 105 +++++++++++++++++++++++++++++++++++++++++++++
 libavutil/frame.h          |   4 ++
 3 files changed, 119 insertions(+)

Comments

Michael Niedermayer Oct. 28, 2017, 2:14 a.m.
On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>  libavcodec/mpegvideo.c     |  10 +++++
>  libavfilter/vf_codecview.c | 105 +++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/frame.h          |   4 ++
>  3 files changed, 119 insertions(+)

First, thanks for working on this.


[...]

> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index fef558ea2f..8481dc080b 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
>       * metadata key entry "name".
>       */
>      AV_FRAME_DATA_ICC_PROFILE,
> +    /**
> +     * Macroblock types exported by some codecs.
> +     */
> +    AV_FRAME_DATA_MACROBLOCK_TYPES,
>  };
>  

This makes the internal data of the decoder part of the ABI and API of
libavcodec and libavfilter
and its undocumented

do people prefer to make the internal data part of the ABI, document
it and ensure it does not change till the next bump
or design a new interface, document it and convert to it?
or is there some other option iam missing ?

these ABI issues btw are why i said i dont have the time for doing the
move.

also it seems the patch breaks fate-mov-zombie

[...]
Ronald S. Bultje Oct. 28, 2017, 11:43 a.m.
Hi,

On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
michael@niedermayer.cc> wrote:

> On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > ---
> >  libavcodec/mpegvideo.c     |  10 +++++
> >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
> +++++++++++++++
> >  libavutil/frame.h          |   4 ++
> >  3 files changed, 119 insertions(+)
>
> First, thanks for working on this.
>
>
> [...]
>
> > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > index fef558ea2f..8481dc080b 100644
> > --- a/libavutil/frame.h
> > +++ b/libavutil/frame.h
> > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
> >       * metadata key entry "name".
> >       */
> >      AV_FRAME_DATA_ICC_PROFILE,
> > +    /**
> > +     * Macroblock types exported by some codecs.
> > +     */
> > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
> >  };
> >
>
> This makes the internal data of the decoder part of the ABI and API of
> libavcodec and libavfilter
> and its undocumented
>
> do people prefer to make the internal data part of the ABI, document
> it and ensure it does not change till the next bump
>
[..]

> or is there some other option iam missing ?
>

I noted this on IRC also. A third option is to not document it and consider
it codec-specific.

(Codec-specific implies "ABI/API unstable" and subject to change - "don't
mix versions".)

> or design a new interface, document it and convert to it?

I think we'd all prefer this, but this requires someone to do the work.
Exciting!

Ronald
Michael Niedermayer Nov. 2, 2017, 11:05 a.m.
Hi

On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
> michael@niedermayer.cc> wrote:
> 
> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > > ---
> > >  libavcodec/mpegvideo.c     |  10 +++++
> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
> > +++++++++++++++
> > >  libavutil/frame.h          |   4 ++
> > >  3 files changed, 119 insertions(+)
> >
> > First, thanks for working on this.
> >
> >
> > [...]
> >
> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > > index fef558ea2f..8481dc080b 100644
> > > --- a/libavutil/frame.h
> > > +++ b/libavutil/frame.h
> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
> > >       * metadata key entry "name".
> > >       */
> > >      AV_FRAME_DATA_ICC_PROFILE,
> > > +    /**
> > > +     * Macroblock types exported by some codecs.
> > > +     */
> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
> > >  };
> > >
> >
> > This makes the internal data of the decoder part of the ABI and API of
> > libavcodec and libavfilter
> > and its undocumented
> >
> > do people prefer to make the internal data part of the ABI, document
> > it and ensure it does not change till the next bump
> >
> [..]
> 
> > or is there some other option iam missing ?
> >
> 
> I noted this on IRC also. A third option is to not document it and consider
> it codec-specific.
> 
> (Codec-specific implies "ABI/API unstable" and subject to change - "don't
> mix versions".)

If you say the interface is "ABI/API unstable" then we cannot use it
from another lib like libavfilter. So this would not work.


> 
> > or design a new interface, document it and convert to it?
> 
> I think we'd all prefer this, but this requires someone to do the work.
> Exciting!

how generic do we want this to be ?
do we have a volunteer to implement this ? because IMO the volunteer
doing the work should be the one primarly deciding how generic to do
this ...

16x16 MB only, or any size ?
first level of subdivision only or multi level ?
only H/V divisions at 50% points or any points, what about divisions by
non 90° angles?
what about non square or non rectangular MBs ?


[...]
Paul B Mahol Nov. 2, 2017, 11:52 a.m.
On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Hi
>
> On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
>> michael@niedermayer.cc> wrote:
>>
>> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
>> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > > ---
>> > >  libavcodec/mpegvideo.c     |  10 +++++
>> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
>> > +++++++++++++++
>> > >  libavutil/frame.h          |   4 ++
>> > >  3 files changed, 119 insertions(+)
>> >
>> > First, thanks for working on this.
>> >
>> >
>> > [...]
>> >
>> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
>> > > index fef558ea2f..8481dc080b 100644
>> > > --- a/libavutil/frame.h
>> > > +++ b/libavutil/frame.h
>> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
>> > >       * metadata key entry "name".
>> > >       */
>> > >      AV_FRAME_DATA_ICC_PROFILE,
>> > > +    /**
>> > > +     * Macroblock types exported by some codecs.
>> > > +     */
>> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
>> > >  };
>> > >
>> >
>> > This makes the internal data of the decoder part of the ABI and API of
>> > libavcodec and libavfilter
>> > and its undocumented
>> >
>> > do people prefer to make the internal data part of the ABI, document
>> > it and ensure it does not change till the next bump
>> >
>> [..]
>>
>> > or is there some other option iam missing ?
>> >
>>
>> I noted this on IRC also. A third option is to not document it and
>> consider
>> it codec-specific.
>>
>> (Codec-specific implies "ABI/API unstable" and subject to change - "don't
>> mix versions".)
>
> If you say the interface is "ABI/API unstable" then we cannot use it
> from another lib like libavfilter. So this would not work.

I disagree.
Ronald S. Bultje Nov. 2, 2017, 4:04 p.m.
Hi,

On Thu, Nov 2, 2017 at 7:52 AM, Paul B Mahol <onemda@gmail.com> wrote:

> On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Hi
> >
> > On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
> >> michael@niedermayer.cc> wrote:
> >>
> >> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> > > ---
> >> > >  libavcodec/mpegvideo.c     |  10 +++++
> >> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
> >> > +++++++++++++++
> >> > >  libavutil/frame.h          |   4 ++
> >> > >  3 files changed, 119 insertions(+)
> >> >
> >> > First, thanks for working on this.
> >> >
> >> >
> >> > [...]
> >> >
> >> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> > > index fef558ea2f..8481dc080b 100644
> >> > > --- a/libavutil/frame.h
> >> > > +++ b/libavutil/frame.h
> >> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
> >> > >       * metadata key entry "name".
> >> > >       */
> >> > >      AV_FRAME_DATA_ICC_PROFILE,
> >> > > +    /**
> >> > > +     * Macroblock types exported by some codecs.
> >> > > +     */
> >> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
> >> > >  };
> >> > >
> >> >
> >> > This makes the internal data of the decoder part of the ABI and API of
> >> > libavcodec and libavfilter
> >> > and its undocumented
> >> >
> >> > do people prefer to make the internal data part of the ABI, document
> >> > it and ensure it does not change till the next bump
> >> >
> >> [..]
> >>
> >> > or is there some other option iam missing ?
> >> >
> >>
> >> I noted this on IRC also. A third option is to not document it and
> >> consider
> >> it codec-specific.
> >>
> >> (Codec-specific implies "ABI/API unstable" and subject to change -
> "don't
> >> mix versions".)
> >
> > If you say the interface is "ABI/API unstable" then we cannot use it
> > from another lib like libavfilter. So this would not work.
>
> I disagree.


I'd like to speak in solutions, not problems. We can simply prepend a
version number to the flags and ensure versions match. That makes it easy
to break "ABI" (and if versions don't match, simply display an error
message and tell the user to not mix versions since this is considered an
unstable feature).

By no means can we set the current ABI of this feature in stone, we're
trying to fix it, not keep it. I appreciate Paul's effort to moving this
forward and think his work pushes it in the right direction.

Ronald
Michael Niedermayer Nov. 8, 2017, 11:06 a.m.
On Thu, Nov 02, 2017 at 12:04:17PM -0400, Ronald S. Bultje wrote:
> Hi,
> 
> On Thu, Nov 2, 2017 at 7:52 AM, Paul B Mahol <onemda@gmail.com> wrote:
> 
> > On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > Hi
> > >
> > > On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
> > >> Hi,
> > >>
> > >> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
> > >> michael@niedermayer.cc> wrote:
> > >>
> > >> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> > >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> > >> > > ---
> > >> > >  libavcodec/mpegvideo.c     |  10 +++++
> > >> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
> > >> > +++++++++++++++
> > >> > >  libavutil/frame.h          |   4 ++
> > >> > >  3 files changed, 119 insertions(+)
> > >> >
> > >> > First, thanks for working on this.
> > >> >
> > >> >
> > >> > [...]
> > >> >
> > >> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
> > >> > > index fef558ea2f..8481dc080b 100644
> > >> > > --- a/libavutil/frame.h
> > >> > > +++ b/libavutil/frame.h
> > >> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
> > >> > >       * metadata key entry "name".
> > >> > >       */
> > >> > >      AV_FRAME_DATA_ICC_PROFILE,
> > >> > > +    /**
> > >> > > +     * Macroblock types exported by some codecs.
> > >> > > +     */
> > >> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
> > >> > >  };
> > >> > >
> > >> >
> > >> > This makes the internal data of the decoder part of the ABI and API of
> > >> > libavcodec and libavfilter
> > >> > and its undocumented
> > >> >
> > >> > do people prefer to make the internal data part of the ABI, document
> > >> > it and ensure it does not change till the next bump
> > >> >
> > >> [..]
> > >>
> > >> > or is there some other option iam missing ?
> > >> >
> > >>
> > >> I noted this on IRC also. A third option is to not document it and
> > >> consider
> > >> it codec-specific.
> > >>
> > >> (Codec-specific implies "ABI/API unstable" and subject to change -
> > "don't
> > >> mix versions".)
> > >
> > > If you say the interface is "ABI/API unstable" then we cannot use it
> > > from another lib like libavfilter. So this would not work.
> >
> > I disagree.
> 
> 
> I'd like to speak in solutions, not problems. We can simply prepend a
> version number to the flags and ensure versions match. That makes it easy
> to break "ABI" (and if versions don't match, simply display an error
> message and tell the user to not mix versions since this is considered an
> unstable feature).
> 
> By no means can we set the current ABI of this feature in stone, we're
> trying to fix it, not keep it. I appreciate Paul's effort to moving this
> forward and think his work pushes it in the right direction.

How does this "versioned", "codec specific" API/ABI fit together with the
carefully designed strict API used by the same filter to access motion
vectors (libavutil/motion_vector.h)
?

[...]
Paul B Mahol Nov. 8, 2017, 11:18 a.m.
On 11/8/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Thu, Nov 02, 2017 at 12:04:17PM -0400, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Thu, Nov 2, 2017 at 7:52 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>
>> > On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > > Hi
>> > >
>> > > On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
>> > >> Hi,
>> > >>
>> > >> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
>> > >> michael@niedermayer.cc> wrote:
>> > >>
>> > >> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
>> > >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> > >> > > ---
>> > >> > >  libavcodec/mpegvideo.c     |  10 +++++
>> > >> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
>> > >> > +++++++++++++++
>> > >> > >  libavutil/frame.h          |   4 ++
>> > >> > >  3 files changed, 119 insertions(+)
>> > >> >
>> > >> > First, thanks for working on this.
>> > >> >
>> > >> >
>> > >> > [...]
>> > >> >
>> > >> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
>> > >> > > index fef558ea2f..8481dc080b 100644
>> > >> > > --- a/libavutil/frame.h
>> > >> > > +++ b/libavutil/frame.h
>> > >> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
>> > >> > >       * metadata key entry "name".
>> > >> > >       */
>> > >> > >      AV_FRAME_DATA_ICC_PROFILE,
>> > >> > > +    /**
>> > >> > > +     * Macroblock types exported by some codecs.
>> > >> > > +     */
>> > >> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
>> > >> > >  };
>> > >> > >
>> > >> >
>> > >> > This makes the internal data of the decoder part of the ABI and API
>> > >> > of
>> > >> > libavcodec and libavfilter
>> > >> > and its undocumented
>> > >> >
>> > >> > do people prefer to make the internal data part of the ABI,
>> > >> > document
>> > >> > it and ensure it does not change till the next bump
>> > >> >
>> > >> [..]
>> > >>
>> > >> > or is there some other option iam missing ?
>> > >> >
>> > >>
>> > >> I noted this on IRC also. A third option is to not document it and
>> > >> consider
>> > >> it codec-specific.
>> > >>
>> > >> (Codec-specific implies "ABI/API unstable" and subject to change -
>> > "don't
>> > >> mix versions".)
>> > >
>> > > If you say the interface is "ABI/API unstable" then we cannot use it
>> > > from another lib like libavfilter. So this would not work.
>> >
>> > I disagree.
>>
>>
>> I'd like to speak in solutions, not problems. We can simply prepend a
>> version number to the flags and ensure versions match. That makes it easy
>> to break "ABI" (and if versions don't match, simply display an error
>> message and tell the user to not mix versions since this is considered an
>> unstable feature).
>>
>> By no means can we set the current ABI of this feature in stone, we're
>> trying to fix it, not keep it. I appreciate Paul's effort to moving this
>> forward and think his work pushes it in the right direction.
>
> How does this "versioned", "codec specific" API/ABI fit together with the
> carefully designed strict API used by the same filter to access motion
> vectors (libavutil/motion_vector.h)
> ?

If you do not like it as it is, write specification which I will follow.
Otherwise this code gonna be removed from decoder soonTM.
Michael Niedermayer Nov. 8, 2017, 5:56 p.m.
On Wed, Nov 08, 2017 at 12:18:24PM +0100, Paul B Mahol wrote:
> On 11/8/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Thu, Nov 02, 2017 at 12:04:17PM -0400, Ronald S. Bultje wrote:
> >> Hi,
> >>
> >> On Thu, Nov 2, 2017 at 7:52 AM, Paul B Mahol <onemda@gmail.com> wrote:
> >>
> >> > On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > > Hi
> >> > >
> >> > > On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
> >> > >> Hi,
> >> > >>
> >> > >> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
> >> > >> michael@niedermayer.cc> wrote:
> >> > >>
> >> > >> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
> >> > >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
> >> > >> > > ---
> >> > >> > >  libavcodec/mpegvideo.c     |  10 +++++
> >> > >> > >  libavfilter/vf_codecview.c | 105 ++++++++++++++++++++++++++++++
> >> > >> > +++++++++++++++
> >> > >> > >  libavutil/frame.h          |   4 ++
> >> > >> > >  3 files changed, 119 insertions(+)
> >> > >> >
> >> > >> > First, thanks for working on this.
> >> > >> >
> >> > >> >
> >> > >> > [...]
> >> > >> >
> >> > >> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
> >> > >> > > index fef558ea2f..8481dc080b 100644
> >> > >> > > --- a/libavutil/frame.h
> >> > >> > > +++ b/libavutil/frame.h
> >> > >> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
> >> > >> > >       * metadata key entry "name".
> >> > >> > >       */
> >> > >> > >      AV_FRAME_DATA_ICC_PROFILE,
> >> > >> > > +    /**
> >> > >> > > +     * Macroblock types exported by some codecs.
> >> > >> > > +     */
> >> > >> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
> >> > >> > >  };
> >> > >> > >
> >> > >> >
> >> > >> > This makes the internal data of the decoder part of the ABI and API
> >> > >> > of
> >> > >> > libavcodec and libavfilter
> >> > >> > and its undocumented
> >> > >> >
> >> > >> > do people prefer to make the internal data part of the ABI,
> >> > >> > document
> >> > >> > it and ensure it does not change till the next bump
> >> > >> >
> >> > >> [..]
> >> > >>
> >> > >> > or is there some other option iam missing ?
> >> > >> >
> >> > >>
> >> > >> I noted this on IRC also. A third option is to not document it and
> >> > >> consider
> >> > >> it codec-specific.
> >> > >>
> >> > >> (Codec-specific implies "ABI/API unstable" and subject to change -
> >> > "don't
> >> > >> mix versions".)
> >> > >
> >> > > If you say the interface is "ABI/API unstable" then we cannot use it
> >> > > from another lib like libavfilter. So this would not work.
> >> >
> >> > I disagree.
> >>
> >>
> >> I'd like to speak in solutions, not problems. We can simply prepend a
> >> version number to the flags and ensure versions match. That makes it easy
> >> to break "ABI" (and if versions don't match, simply display an error
> >> message and tell the user to not mix versions since this is considered an
> >> unstable feature).
> >>
> >> By no means can we set the current ABI of this feature in stone, we're
> >> trying to fix it, not keep it. I appreciate Paul's effort to moving this
> >> forward and think his work pushes it in the right direction.
> >
> > How does this "versioned", "codec specific" API/ABI fit together with the
> > carefully designed strict API used by the same filter to access motion
> > vectors (libavutil/motion_vector.h)
> > ?
> 
> If you do not like it as it is, write specification which I will follow.

I can. Is there some preferrances that people have ?
this can be done in a wide range from simple to quite complex.
from a simple bitmask in a byte or int array to a tree structure
describing subdivisions or a list of structs similar to
libavutil/motion_vector.h.
The simple one will not be able to represent everything


> Otherwise this code gonna be removed from decoder soonTM.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Paul B Mahol Nov. 8, 2017, 7:06 p.m.
On 11/8/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Wed, Nov 08, 2017 at 12:18:24PM +0100, Paul B Mahol wrote:
>> On 11/8/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Thu, Nov 02, 2017 at 12:04:17PM -0400, Ronald S. Bultje wrote:
>> >> Hi,
>> >>
>> >> On Thu, Nov 2, 2017 at 7:52 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> >>
>> >> > On 11/2/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > > Hi
>> >> > >
>> >> > > On Sat, Oct 28, 2017 at 07:43:05AM -0400, Ronald S. Bultje wrote:
>> >> > >> Hi,
>> >> > >>
>> >> > >> On Fri, Oct 27, 2017 at 10:14 PM, Michael Niedermayer <
>> >> > >> michael@niedermayer.cc> wrote:
>> >> > >>
>> >> > >> > On Fri, Oct 27, 2017 at 10:03:54PM +0200, Paul B Mahol wrote:
>> >> > >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> >> > >> > > ---
>> >> > >> > >  libavcodec/mpegvideo.c     |  10 +++++
>> >> > >> > >  libavfilter/vf_codecview.c | 105
>> >> > >> > > ++++++++++++++++++++++++++++++
>> >> > >> > +++++++++++++++
>> >> > >> > >  libavutil/frame.h          |   4 ++
>> >> > >> > >  3 files changed, 119 insertions(+)
>> >> > >> >
>> >> > >> > First, thanks for working on this.
>> >> > >> >
>> >> > >> >
>> >> > >> > [...]
>> >> > >> >
>> >> > >> > > diff --git a/libavutil/frame.h b/libavutil/frame.h
>> >> > >> > > index fef558ea2f..8481dc080b 100644
>> >> > >> > > --- a/libavutil/frame.h
>> >> > >> > > +++ b/libavutil/frame.h
>> >> > >> > > @@ -141,6 +141,10 @@ enum AVFrameSideDataType {
>> >> > >> > >       * metadata key entry "name".
>> >> > >> > >       */
>> >> > >> > >      AV_FRAME_DATA_ICC_PROFILE,
>> >> > >> > > +    /**
>> >> > >> > > +     * Macroblock types exported by some codecs.
>> >> > >> > > +     */
>> >> > >> > > +    AV_FRAME_DATA_MACROBLOCK_TYPES,
>> >> > >> > >  };
>> >> > >> > >
>> >> > >> >
>> >> > >> > This makes the internal data of the decoder part of the ABI and
>> >> > >> > API
>> >> > >> > of
>> >> > >> > libavcodec and libavfilter
>> >> > >> > and its undocumented
>> >> > >> >
>> >> > >> > do people prefer to make the internal data part of the ABI,
>> >> > >> > document
>> >> > >> > it and ensure it does not change till the next bump
>> >> > >> >
>> >> > >> [..]
>> >> > >>
>> >> > >> > or is there some other option iam missing ?
>> >> > >> >
>> >> > >>
>> >> > >> I noted this on IRC also. A third option is to not document it
>> >> > >> and
>> >> > >> consider
>> >> > >> it codec-specific.
>> >> > >>
>> >> > >> (Codec-specific implies "ABI/API unstable" and subject to change
>> >> > >> -
>> >> > "don't
>> >> > >> mix versions".)
>> >> > >
>> >> > > If you say the interface is "ABI/API unstable" then we cannot use
>> >> > > it
>> >> > > from another lib like libavfilter. So this would not work.
>> >> >
>> >> > I disagree.
>> >>
>> >>
>> >> I'd like to speak in solutions, not problems. We can simply prepend a
>> >> version number to the flags and ensure versions match. That makes it
>> >> easy
>> >> to break "ABI" (and if versions don't match, simply display an error
>> >> message and tell the user to not mix versions since this is considered
>> >> an
>> >> unstable feature).
>> >>
>> >> By no means can we set the current ABI of this feature in stone, we're
>> >> trying to fix it, not keep it. I appreciate Paul's effort to moving
>> >> this
>> >> forward and think his work pushes it in the right direction.
>> >
>> > How does this "versioned", "codec specific" API/ABI fit together with
>> > the
>> > carefully designed strict API used by the same filter to access motion
>> > vectors (libavutil/motion_vector.h)
>> > ?
>>
>> If you do not like it as it is, write specification which I will follow.
>
> I can. Is there some preferrances that people have ?
> this can be done in a wide range from simple to quite complex.
> from a simple bitmask in a byte or int array to a tree structure
> describing subdivisions or a list of structs similar to
> libavutil/motion_vector.h.
> The simple one will not be able to represent everything

Just one that gonna be used by single codec.
More complex one can be added when needed.

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
index 2f5793b9a4..ce6108d094 100644
--- a/libavcodec/mpegvideo.c
+++ b/libavcodec/mpegvideo.c
@@ -1603,6 +1603,16 @@  void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict, uint8_t *mbskip_
         }
     }
 
+    if (mb_height && mb_width) {
+        AVFrameSideData *sd;
+
+        av_log(avctx, AV_LOG_DEBUG, "Adding %d MB types info to frame %d\n", mb_width * mb_height, avctx->frame_number);
+        sd = av_frame_new_side_data(pict, AV_FRAME_DATA_MACROBLOCK_TYPES, mb_width * mb_height * sizeof(uint32_t));
+        if (!sd)
+            return;
+        memcpy(sd->data, mbtype_table, mb_width * mb_height * sizeof(uint32_t));
+    }
+
 #if FF_API_DEBUG_MV
     if ((avctx->debug & (FF_DEBUG_VIS_QP | FF_DEBUG_VIS_MB_TYPE)) ||
         (avctx->debug_mv)) {
diff --git a/libavfilter/vf_codecview.c b/libavfilter/vf_codecview.c
index 331bfba777..40fb8dfb7a 100644
--- a/libavfilter/vf_codecview.c
+++ b/libavfilter/vf_codecview.c
@@ -29,6 +29,7 @@ 
  * TODO: segmentation
  */
 
+#include "libavutil/avassert.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/motion_vector.h"
 #include "libavutil/opt.h"
@@ -44,9 +45,23 @@ 
 #define FRAME_TYPE_P (1<<1)
 #define FRAME_TYPE_B (1<<2)
 
+#define IS_PCM(a)           ((a) & (1 << 2))
+#define IS_INTRA(a)         ((a) & 7)
+#define IS_ACPRED(a)        ((a) & (1 << 9))
+#define IS_INTRA16x16(a)    ((a) & (1 << 1))
+#define IS_INTRA4x4(a)      ((a) & (1 << 0))
+#define IS_DIRECT(a)        ((a) & (1 << 8))
+#define IS_SKIP(a)          ((a) & (1 << 11))
+#define IS_GMC(a)           ((a) & (1 << 10))
+#define USES_LIST(a, list)  ((a) & (((1 << 12) | (1 << 13)) << (2 * (list))))
+#define IS_8X8(a)           ((a) & ((1 << 6)))
+#define IS_16X8(a)          ((a) & ((1 << 4)))
+#define IS_8X16(a)          ((a) & ((1 << 5)))
+
 typedef struct CodecViewContext {
     const AVClass *class;
     unsigned mv;
+    unsigned mbtypes;
     unsigned frame_type;
     unsigned mv_type;
     int hsub, vsub;
@@ -72,6 +87,7 @@  static const AVOption codecview_options[] = {
         CONST("if", "I-frames", FRAME_TYPE_I, "frame_type"),
         CONST("pf", "P-frames", FRAME_TYPE_P, "frame_type"),
         CONST("bf", "B-frames", FRAME_TYPE_B, "frame_type"),
+    { "mb", "visualize macroblock types", OFFSET(mbtypes), AV_OPT_TYPE_BOOL, {.i64=0}, 0, 1, FLAGS, },
     { NULL }
 };
 
@@ -277,6 +293,95 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
         }
     }
 
+    if (s->mbtypes) {
+        AVFrameSideData *sd = av_frame_get_side_data(frame, AV_FRAME_DATA_MACROBLOCK_TYPES);
+        if (sd) {
+            int mb_height = (frame->height + 31) / 32 * 2;
+            int mb_width = (frame->width + 15) / 16;
+            int block_height = 16 >> 1;
+            int mb_stride = mb_width + 1;
+            int mb_y, mb_x;
+            uint32_t *mbtype_table = (uint32_t *)sd->data;
+
+            for (mb_y = 0; mb_y < mb_height; mb_y++) {
+                for (mb_x = 0; mb_x < mb_width; mb_x++) {
+                    const int mb_index = mb_x + mb_y * mb_stride;
+                  /*{
+                        uint64_t c = (qscale_table[mb_index] * 128 / 31) *
+                                 0x0101010101010101ULL;
+                        int y;
+                        for (y = 0; y < block_height; y++) {
+                            *(uint64_t *)(frame->data[1] + 8 * mb_x +
+                                          (block_height * mb_y + y) *
+                                          frame->linesize[1]) = c;
+                            *(uint64_t *)(frame->data[2] + 8 * mb_x +
+                                          (block_height * mb_y + y) *
+                                          frame->linesize[2]) = c;
+                        }
+                    } */
+                    {
+                        int mb_type = mbtype_table[mb_index];
+                        uint64_t u,v;
+                        int y;
+
+#define COLOR(theta, r) \
+    u = (int)(128 + r * cos(theta * M_PI / 180)); \
+    v = (int)(128 + r * sin(theta * M_PI / 180));
+
+                        u = v = 128;
+                        if (IS_PCM(mb_type)) {
+                            COLOR(120, 48)
+                        } else if ((IS_INTRA(mb_type) && IS_ACPRED(mb_type)) ||
+                                   IS_INTRA16x16(mb_type)) {
+                            COLOR(30, 48)
+                        } else if (IS_INTRA4x4(mb_type)) {
+                            COLOR(90, 48)
+                        } else if (IS_DIRECT(mb_type) && IS_SKIP(mb_type)) {
+                            COLOR(120, 48)
+                        } else if (IS_DIRECT(mb_type)) {
+                            COLOR(150, 48)
+                        } else if (IS_GMC(mb_type) && IS_SKIP(mb_type)) {
+                            COLOR(170, 48)
+                        } else if (IS_GMC(mb_type)) {
+                            COLOR(190, 48)
+                        } else if (IS_SKIP(mb_type)) {
+                            COLOR(180, 48)
+                        } else if (!USES_LIST(mb_type, 1)) {
+                            COLOR(240, 48)
+                        } else if (!USES_LIST(mb_type, 0)) {
+                            COLOR(0, 48)
+                        } else {
+                            av_assert2(USES_LIST(mb_type, 0) && USES_LIST(mb_type, 1));
+                            COLOR(300,48)
+                        }
+
+                        u *= 0x0101010101010101ULL;
+                        v *= 0x0101010101010101ULL;
+                        for (y = 0; y < block_height; y++) {
+                            *(uint64_t *)(frame->data[1] + 8 * mb_x +
+                                          (block_height * mb_y + y) * frame->linesize[1]) = u;
+                            *(uint64_t *)(frame->data[2] + 8 * mb_x +
+                                          (block_height * mb_y + y) * frame->linesize[2]) = v;
+                        }
+
+                        // segmentation
+                        if (IS_8X8(mb_type) || IS_16X8(mb_type)) {
+                            *(uint64_t *)(frame->data[0] + 16 * mb_x + 0 +
+                                          (16 * mb_y + 8) * frame->linesize[0]) ^= 0x8080808080808080ULL;
+                            *(uint64_t *)(frame->data[0] + 16 * mb_x + 8 +
+                                          (16 * mb_y + 8) * frame->linesize[0]) ^= 0x8080808080808080ULL;
+                        }
+                        if (IS_8X8(mb_type) || IS_8X16(mb_type)) {
+                            for (y = 0; y < 16; y++)
+                                frame->data[0][16 * mb_x + 8 + (16 * mb_y + y) *
+                                              frame->linesize[0]] ^= 0x80;
+                        }
+                    }
+                }
+            }
+        }
+    }
+
     return ff_filter_frame(outlink, frame);
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index fef558ea2f..8481dc080b 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -141,6 +141,10 @@  enum AVFrameSideDataType {
      * metadata key entry "name".
      */
     AV_FRAME_DATA_ICC_PROFILE,
+    /**
+     * Macroblock types exported by some codecs.
+     */
+    AV_FRAME_DATA_MACROBLOCK_TYPES,
 };
 
 enum AVActiveFormatDescription {