diff mbox

[FFmpeg-devel,libdav1d.c] : Add option to output all the spatial layers.

Message ID 20191114171534.258181-1-tfoucu@gmail.com
State New
Headers show

Commit Message

Thierry Foucu Nov. 14, 2019, 5:15 p.m. UTC
Set the option to false by default, to match libaomdec wrapper.
---
 libavcodec/libdav1d.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

James Almer Nov. 14, 2019, 5:19 p.m. UTC | #1
On 11/14/2019 2:15 PM, Thierry Foucu wrote:
> Set the option to false by default, to match libaomdec wrapper.
> ---
>  libavcodec/libdav1d.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
> index cf4b178f1d..5efa8eeb48 100644
> --- a/libavcodec/libdav1d.c
> +++ b/libavcodec/libdav1d.c
> @@ -40,6 +40,7 @@ typedef struct Libdav1dContext {
>      int tile_threads;
>      int frame_threads;
>      int apply_grain;
> +    int all_layers;
>  } Libdav1dContext;
>  
>  static const enum AVPixelFormat pix_fmt[][3] = {
> @@ -134,6 +135,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
>      if (dav1d->apply_grain >= 0)
>          s.apply_grain = dav1d->apply_grain;
>  
> +    s.all_layers = dav1d->all_layers;
> +
>      s.n_tile_threads = dav1d->tile_threads
>                       ? dav1d->tile_threads
>                       : FFMIN(floor(sqrt(threads)), DAV1D_MAX_TILE_THREADS);
> @@ -378,6 +381,7 @@ static const AVOption libdav1d_options[] = {
>      { "tilethreads", "Tile threads", OFFSET(tile_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
>      { "framethreads", "Frame threads", OFFSET(frame_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
>      { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
> +    { "alllayers", "Output all spatial layers", OFFSET(all_layers), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
>      { NULL }
>  };

IMO, we don't want to output all layers under any circumstance. It'll
result in a mix of frames with duplicate pts and different dimensions.

What you could add is a new option "oppoint" that maps to Dav1dSettings
operating_point. Set it to -1, and handle it the same way as filmgrain
in libdav1d_init(), and then unconditionally set all_layers to 0.
Thierry Foucu Nov. 14, 2019, 5:23 p.m. UTC | #2
On Thu, Nov 14, 2019 at 9:20 AM James Almer <jamrial@gmail.com> wrote:

> On 11/14/2019 2:15 PM, Thierry Foucu wrote:
> > Set the option to false by default, to match libaomdec wrapper.
> > ---
> >  libavcodec/libdav1d.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
> > index cf4b178f1d..5efa8eeb48 100644
> > --- a/libavcodec/libdav1d.c
> > +++ b/libavcodec/libdav1d.c
> > @@ -40,6 +40,7 @@ typedef struct Libdav1dContext {
> >      int tile_threads;
> >      int frame_threads;
> >      int apply_grain;
> > +    int all_layers;
> >  } Libdav1dContext;
> >
> >  static const enum AVPixelFormat pix_fmt[][3] = {
> > @@ -134,6 +135,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
> >      if (dav1d->apply_grain >= 0)
> >          s.apply_grain = dav1d->apply_grain;
> >
> > +    s.all_layers = dav1d->all_layers;
> > +
> >      s.n_tile_threads = dav1d->tile_threads
> >                       ? dav1d->tile_threads
> >                       : FFMIN(floor(sqrt(threads)),
> DAV1D_MAX_TILE_THREADS);
> > @@ -378,6 +381,7 @@ static const AVOption libdav1d_options[] = {
> >      { "tilethreads", "Tile threads", OFFSET(tile_threads),
> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
> >      { "framethreads", "Frame threads", OFFSET(frame_threads),
> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
> >      { "filmgrain", "Apply Film Grain", OFFSET(apply_grain),
> AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
> > +    { "alllayers", "Output all spatial layers", OFFSET(all_layers),
> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
> >      { NULL }
> >  };
>
> IMO, we don't want to output all layers under any circumstance. It'll
> result in a mix of frames with duplicate pts and different dimensions.
>
> What you could add is a new option "oppoint" that maps to Dav1dSettings
> operating_point. Set it to -1, and handle it the same way as filmgrain
> in libdav1d_init(), and then unconditionally set all_layers to 0.
>

I think to have an option to output all the layers is still a good idea if
you need to debug some layers issues.
especially  if you need to run some conformance testing and debug each
layer.

But i will add the other option to select the layer.


> _______________________________________________
> 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".
Andrey Semashev Nov. 14, 2019, 5:31 p.m. UTC | #3
On 2019-11-14 20:19, James Almer wrote:
> On 11/14/2019 2:15 PM, Thierry Foucu wrote:
>> Set the option to false by default, to match libaomdec wrapper.
>> ---
>>   libavcodec/libdav1d.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
>> index cf4b178f1d..5efa8eeb48 100644
>> --- a/libavcodec/libdav1d.c
>> +++ b/libavcodec/libdav1d.c
>> @@ -40,6 +40,7 @@ typedef struct Libdav1dContext {
>>       int tile_threads;
>>       int frame_threads;
>>       int apply_grain;
>> +    int all_layers;
>>   } Libdav1dContext;
>>   
>>   static const enum AVPixelFormat pix_fmt[][3] = {
>> @@ -134,6 +135,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
>>       if (dav1d->apply_grain >= 0)
>>           s.apply_grain = dav1d->apply_grain;
>>   
>> +    s.all_layers = dav1d->all_layers;
>> +
>>       s.n_tile_threads = dav1d->tile_threads
>>                        ? dav1d->tile_threads
>>                        : FFMIN(floor(sqrt(threads)), DAV1D_MAX_TILE_THREADS);
>> @@ -378,6 +381,7 @@ static const AVOption libdav1d_options[] = {
>>       { "tilethreads", "Tile threads", OFFSET(tile_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
>>       { "framethreads", "Frame threads", OFFSET(frame_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
>>       { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
>> +    { "alllayers", "Output all spatial layers", OFFSET(all_layers), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
>>       { NULL }
>>   };
> 
> IMO, we don't want to output all layers under any circumstance. It'll
> result in a mix of frames with duplicate pts and different dimensions.

IIRC, for VP9 with spatial SVC lavc decoder worked exactly like that - 
by returning a frame per spatial layer. Is this considered a bug?
James Almer Nov. 14, 2019, 5:48 p.m. UTC | #4
On 11/14/2019 2:31 PM, Andrey Semashev wrote:
> On 2019-11-14 20:19, James Almer wrote:
>> On 11/14/2019 2:15 PM, Thierry Foucu wrote:
>>> Set the option to false by default, to match libaomdec wrapper.
>>> ---
>>>   libavcodec/libdav1d.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
>>> index cf4b178f1d..5efa8eeb48 100644
>>> --- a/libavcodec/libdav1d.c
>>> +++ b/libavcodec/libdav1d.c
>>> @@ -40,6 +40,7 @@ typedef struct Libdav1dContext {
>>>       int tile_threads;
>>>       int frame_threads;
>>>       int apply_grain;
>>> +    int all_layers;
>>>   } Libdav1dContext;
>>>     static const enum AVPixelFormat pix_fmt[][3] = {
>>> @@ -134,6 +135,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
>>>       if (dav1d->apply_grain >= 0)
>>>           s.apply_grain = dav1d->apply_grain;
>>>   +    s.all_layers = dav1d->all_layers;
>>> +
>>>       s.n_tile_threads = dav1d->tile_threads
>>>                        ? dav1d->tile_threads
>>>                        : FFMIN(floor(sqrt(threads)),
>>> DAV1D_MAX_TILE_THREADS);
>>> @@ -378,6 +381,7 @@ static const AVOption libdav1d_options[] = {
>>>       { "tilethreads", "Tile threads", OFFSET(tile_threads),
>>> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
>>>       { "framethreads", "Frame threads", OFFSET(frame_threads),
>>> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
>>>       { "filmgrain", "Apply Film Grain", OFFSET(apply_grain),
>>> AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
>>> +    { "alllayers", "Output all spatial layers", OFFSET(all_layers),
>>> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
>>>       { NULL }
>>>   };
>>
>> IMO, we don't want to output all layers under any circumstance. It'll
>> result in a mix of frames with duplicate pts and different dimensions.
> 
> IIRC, for VP9 with spatial SVC lavc decoder worked exactly like that -
> by returning a frame per spatial layer. Is this considered a bug?

I don't know if it's a bug, and never seen or tested vp9 svc samples
before. Ronald might know. But it sounds like having more than one video
"stream" per AVCodecContext, which i don't think was intended.
And if it was, then it would be up to the library user to figure what to
do with these frames.
Andrey Semashev Nov. 14, 2019, 7:06 p.m. UTC | #5
On 2019-11-14 20:48, James Almer wrote:
> On 11/14/2019 2:31 PM, Andrey Semashev wrote:
>> On 2019-11-14 20:19, James Almer wrote:
>>> On 11/14/2019 2:15 PM, Thierry Foucu wrote:
>>>> Set the option to false by default, to match libaomdec wrapper.
>>>> ---
>>>>    libavcodec/libdav1d.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
>>>> index cf4b178f1d..5efa8eeb48 100644
>>>> --- a/libavcodec/libdav1d.c
>>>> +++ b/libavcodec/libdav1d.c
>>>> @@ -40,6 +40,7 @@ typedef struct Libdav1dContext {
>>>>        int tile_threads;
>>>>        int frame_threads;
>>>>        int apply_grain;
>>>> +    int all_layers;
>>>>    } Libdav1dContext;
>>>>      static const enum AVPixelFormat pix_fmt[][3] = {
>>>> @@ -134,6 +135,8 @@ static av_cold int libdav1d_init(AVCodecContext *c)
>>>>        if (dav1d->apply_grain >= 0)
>>>>            s.apply_grain = dav1d->apply_grain;
>>>>    +    s.all_layers = dav1d->all_layers;
>>>> +
>>>>        s.n_tile_threads = dav1d->tile_threads
>>>>                         ? dav1d->tile_threads
>>>>                         : FFMIN(floor(sqrt(threads)),
>>>> DAV1D_MAX_TILE_THREADS);
>>>> @@ -378,6 +381,7 @@ static const AVOption libdav1d_options[] = {
>>>>        { "tilethreads", "Tile threads", OFFSET(tile_threads),
>>>> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
>>>>        { "framethreads", "Frame threads", OFFSET(frame_threads),
>>>> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
>>>>        { "filmgrain", "Apply Film Grain", OFFSET(apply_grain),
>>>> AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
>>>> +    { "alllayers", "Output all spatial layers", OFFSET(all_layers),
>>>> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
>>>>        { NULL }
>>>>    };
>>>
>>> IMO, we don't want to output all layers under any circumstance. It'll
>>> result in a mix of frames with duplicate pts and different dimensions.
>>
>> IIRC, for VP9 with spatial SVC lavc decoder worked exactly like that -
>> by returning a frame per spatial layer. Is this considered a bug?
> 
> I don't know if it's a bug, and never seen or tested vp9 svc samples
> before. Ronald might know. But it sounds like having more than one video
> "stream" per AVCodecContext, which i don't think was intended.

There is one stream, in which an encoded frame is a VP9 superframe that 
contains frames for each spatial layer. From the user's perspective, VP9 
superframe is one entity.

> And if it was, then it would be up to the library user to figure what to
> do with these frames.

I think there needs to be some consistency across different lavc 
decoders. If we consider that lavc should produce one decoded frame per 
one encoded one, even if the encoded one contains multiple layers, then 
that should be true for all decoders.

Also, I think having decoded frames from all layers would also be 
useful, but there should be a way to know which layer they belong to. 
AFAIK, lavc currently doesn't provide that information. This mode of 
operation (producing frames for all layers) should be optional.
Ronald S. Bultje Nov. 14, 2019, 7:33 p.m. UTC | #6
Hi,

On Thu, Nov 14, 2019 at 2:12 PM Andrey Semashev <andrey.semashev@gmail.com>
wrote:

> I think there needs to be some consistency across different lavc
> decoders. If we consider that lavc should produce one decoded frame per
> one encoded one, even if the encoded one contains multiple layers, then
> that should be true for all decoders.
>

Yes. I think one thing that would help is if we had access to more samples
with an expected behaviour. Right now we may have samples, but if all we do
is check md5 without caring what it means, then it's kind of pointless.


> Also, I think having decoded frames from all layers would also be
> useful, but there should be a way to know which layer they belong to.
> AFAIK, lavc currently doesn't provide that information. This mode of
> operation (producing frames for all layers) should be optional.


I agree.

Ronald
Thierry Foucu Nov. 14, 2019, 8:09 p.m. UTC | #7
On Thu, Nov 14, 2019 at 11:33 AM Ronald S. Bultje <rsbultje@gmail.com>
wrote:

> Hi,
>
> On Thu, Nov 14, 2019 at 2:12 PM Andrey Semashev <andrey.semashev@gmail.com
> >
> wrote:
>
> > I think there needs to be some consistency across different lavc
> > decoders. If we consider that lavc should produce one decoded frame per
> > one encoded one, even if the encoded one contains multiple layers, then
> > that should be true for all decoders.
> >
>
> Yes. I think one thing that would help is if we had access to more samples
> with an expected behaviour. Right now we may have samples, but if all we do
> is check md5 without caring what it means, then it's kind of pointless.
>
>
> > Also, I think having decoded frames from all layers would also be
> > useful, but there should be a way to know which layer they belong to.
> > AFAIK, lavc currently doesn't provide that information. This mode of
> > operation (producing frames for all layers) should be optional.
>
>
> I agree.
>

I think this is what my second patch is doing.
Set by default to false to output all the layer and add an option to select
which operating point we want the decoder to output.


>
> Ronald
> _______________________________________________
> 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".
diff mbox

Patch

diff --git a/libavcodec/libdav1d.c b/libavcodec/libdav1d.c
index cf4b178f1d..5efa8eeb48 100644
--- a/libavcodec/libdav1d.c
+++ b/libavcodec/libdav1d.c
@@ -40,6 +40,7 @@  typedef struct Libdav1dContext {
     int tile_threads;
     int frame_threads;
     int apply_grain;
+    int all_layers;
 } Libdav1dContext;
 
 static const enum AVPixelFormat pix_fmt[][3] = {
@@ -134,6 +135,8 @@  static av_cold int libdav1d_init(AVCodecContext *c)
     if (dav1d->apply_grain >= 0)
         s.apply_grain = dav1d->apply_grain;
 
+    s.all_layers = dav1d->all_layers;
+
     s.n_tile_threads = dav1d->tile_threads
                      ? dav1d->tile_threads
                      : FFMIN(floor(sqrt(threads)), DAV1D_MAX_TILE_THREADS);
@@ -378,6 +381,7 @@  static const AVOption libdav1d_options[] = {
     { "tilethreads", "Tile threads", OFFSET(tile_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_TILE_THREADS, VD },
     { "framethreads", "Frame threads", OFFSET(frame_threads), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, DAV1D_MAX_FRAME_THREADS, VD },
     { "filmgrain", "Apply Film Grain", OFFSET(apply_grain), AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, VD },
+    { "alllayers", "Output all spatial layers", OFFSET(all_layers), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VD },
     { NULL }
 };