[FFmpeg-devel,v2,3/3] lavc/libdavs2: correct frame type setting

Submitted by hwren on Oct. 31, 2018, 10:23 a.m.

Details

Message ID 1540981431-6995-3-git-send-email-hwrenx@126.com
State New
Headers show

Commit Message

hwren Oct. 31, 2018, 10:23 a.m.
Signed-off-by: hwren <hwrenx@126.com>
---
 libavcodec/libdavs2.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Oct. 31, 2018, 12:55 p.m.
2018-10-31 11:23 GMT+01:00, hwren <hwrenx@126.com>:
> Signed-off-by: hwren <hwrenx@126.com>
> ---
>  libavcodec/libdavs2.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c
> index a1815d2..d7bcaa3 100644
> --- a/libavcodec/libdavs2.c
> +++ b/libavcodec/libdavs2.c
> @@ -94,11 +94,26 @@ static int davs2_dump_frames(AVCodecContext *avctx,
> davs2_picture_t *pic,
>                     pic->widths[plane] * bytes_per_sample);
>      }
>
> +    switch (pic->type) {
> +        case DAVS2_PIC_I:
> +            frame->pict_type = AV_PICTURE_TYPE_I;
> +            break;
> +        case DAVS2_PIC_P:
> +            frame->pict_type = AV_PICTURE_TYPE_P;
> +            break;
> +        case DAVS2_PIC_B:
> +        case DAVS2_PIC_F:
> +            frame->pict_type = AV_PICTURE_TYPE_B;
> +            break;

> +        default:
> +            frame->pict_type = AV_PICTURE_TYPE_NONE;

What other DAVS2_PIC values exist?

Carl Eugen
Mark Thompson Oct. 31, 2018, 10:15 p.m.
On 31/10/18 10:23, hwren wrote:
> Signed-off-by: hwren <hwrenx@126.com>
> ---
>  libavcodec/libdavs2.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c
> index a1815d2..d7bcaa3 100644
> --- a/libavcodec/libdavs2.c
> +++ b/libavcodec/libdavs2.c
> @@ -94,11 +94,26 @@ static int davs2_dump_frames(AVCodecContext *avctx, davs2_picture_t *pic,
>                     pic->widths[plane] * bytes_per_sample);
>      }
>  
> +    switch (pic->type) {
> +        case DAVS2_PIC_I:
> +            frame->pict_type = AV_PICTURE_TYPE_I;
> +            break;
> +        case DAVS2_PIC_P:
> +            frame->pict_type = AV_PICTURE_TYPE_P;
> +            break;
> +        case DAVS2_PIC_B:
> +        case DAVS2_PIC_F:
> +            frame->pict_type = AV_PICTURE_TYPE_B;
> +            break;
> +        default:
> +            frame->pict_type = AV_PICTURE_TYPE_NONE;

Are there any types which aren't already handled?  If there aren't then this would probably be better as an assert.

> +    }
> +
>      frame->width     = cad->headerset.width;
>      frame->height    = cad->headerset.height;
>      frame->pts       = cad->out_frame.pts;
> -    frame->pict_type = pic->type;
>      frame->format    = avctx->pix_fmt;
> +    frame->key_frame = pic->type == DAVS2_PIC_I ? 1 : 0;

Is an I frame in AVS2 necessarily a random access point?  (That is, not like an I frame in H.264, instead like an IDR frame - it clears the DPB so no previous reference frames can be carried across it.)

>  
>      return 1;
>  }
> 

Thanks

- Mark
Carl Eugen Hoyos Nov. 1, 2018, 12:40 a.m.
2018-10-31 23:15 GMT+01:00, Mark Thompson <sw@jkqxz.net>:
> On 31/10/18 10:23, hwren wrote:

>> +    switch (pic->type) {
>> +        case DAVS2_PIC_I:
>> +            frame->pict_type = AV_PICTURE_TYPE_I;
>> +            break;
>> +        case DAVS2_PIC_P:
>> +            frame->pict_type = AV_PICTURE_TYPE_P;
>> +            break;
>> +        case DAVS2_PIC_B:
>> +        case DAVS2_PIC_F:
>> +            frame->pict_type = AV_PICTURE_TYPE_B;
>> +            break;
>> +        default:
>> +            frame->pict_type = AV_PICTURE_TYPE_NONE;
>
> Are there any types which aren't already handled?  If there aren't
> then this would probably be better as an assert.

Since this is an external library, an assert would be wrong.

I wanted to suggest an error message or a negative return value.

Carl Eugen
hwren Nov. 1, 2018, 3:19 a.m.
At 2018-11-01 08:40:50, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:
>2018-10-31 23:15 GMT+01:00, Mark Thompson <sw@jkqxz.net>:
>> On 31/10/18 10:23, hwren wrote:
>
>>> +    switch (pic->type) {
>>> +        case DAVS2_PIC_I:
>>> +            frame->pict_type = AV_PICTURE_TYPE_I;
>>> +            break;
>>> +        case DAVS2_PIC_P:
>>> +            frame->pict_type = AV_PICTURE_TYPE_P;
>>> +            break;
>>> +        case DAVS2_PIC_B:
>>> +        case DAVS2_PIC_F:
>>> +            frame->pict_type = AV_PICTURE_TYPE_B;
>>> +            break;
>>> +        default:
>>> +            frame->pict_type = AV_PICTURE_TYPE_NONE;
>>
>> Are there any types which aren't already handled?  If there aren't
>> then this would probably be better as an assert.
>
>Since this is an external library, an assert would be wrong.
>
>I wanted to suggest an error message or a negative return value.

Actually, there are six types of frames defined by AVS2 standard: 

DAVS2_PIC_I , DAVS2_PIC_P, DAVS2_PIC_B
DAVS2_PIC_G, DAVS2_PIC_F ,DAVS2_PIC_S
(the DAVS2_PIC_S is not exactly the same thing as AV_PICTURE_TYPE_S)

While in this patch I may make a wrong classification. Every frame in davs2 has its same/similar type in ffmpeg.
So maybe it's better to set the  unhandled-type to an error assert.

Thanks :-)

>
>Carl Eugen
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel@ffmpeg.org
>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

diff --git a/libavcodec/libdavs2.c b/libavcodec/libdavs2.c
index a1815d2..d7bcaa3 100644
--- a/libavcodec/libdavs2.c
+++ b/libavcodec/libdavs2.c
@@ -94,11 +94,26 @@  static int davs2_dump_frames(AVCodecContext *avctx, davs2_picture_t *pic,
                    pic->widths[plane] * bytes_per_sample);
     }
 
+    switch (pic->type) {
+        case DAVS2_PIC_I:
+            frame->pict_type = AV_PICTURE_TYPE_I;
+            break;
+        case DAVS2_PIC_P:
+            frame->pict_type = AV_PICTURE_TYPE_P;
+            break;
+        case DAVS2_PIC_B:
+        case DAVS2_PIC_F:
+            frame->pict_type = AV_PICTURE_TYPE_B;
+            break;
+        default:
+            frame->pict_type = AV_PICTURE_TYPE_NONE;
+    }
+
     frame->width     = cad->headerset.width;
     frame->height    = cad->headerset.height;
     frame->pts       = cad->out_frame.pts;
-    frame->pict_type = pic->type;
     frame->format    = avctx->pix_fmt;
+    frame->key_frame = pic->type == DAVS2_PIC_I ? 1 : 0;
 
     return 1;
 }