diff mbox

[FFmpeg-devel,13/17] lavc/mjpeg: Add profiles for MJPEG using SOF marker codes

Message ID 20171124005134.5683-13-sw@jkqxz.net
State Superseded
Headers show

Commit Message

Mark Thompson Nov. 24, 2017, 12:51 a.m. UTC 
This is needed by later hwaccel code to tell which encoding process was
used for a particular frame, because hardware decoders may only support a
subset of possible methods.
---
 libavcodec/avcodec.h  | 6 ++++++
 libavcodec/mjpegdec.c | 7 +++++++
 2 files changed, 13 insertions(+)

Comments

Carl Eugen Hoyos Nov. 24, 2017, 11:27 a.m. UTC | #1 
2017-11-24 1:51 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> This is needed by later hwaccel code to tell which encoding process was
> used for a particular frame, because hardware decoders may only support a
> subset of possible methods.
> ---
>  libavcodec/avcodec.h  | 6 ++++++
>  libavcodec/mjpegdec.c | 7 +++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 0972df0bde..c1e68b1d13 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2897,6 +2897,12 @@ typedef struct AVCodecContext {
>  #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
>  #define FF_PROFILE_HEVC_REXT                        4
>
> +#define FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT            0xc0
> +#define FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT 0xc1
> +#define FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT         0xc2
> +#define FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS                0xc3
> +#define FF_PROFILE_MJPEG_JPEG_LS                         0xf7

Sorry if I misread the code:
If you (thankfully) define these as in the specification, why don't you
simply set them without the "if()"'s?

Carl Eugen
Mark Thompson Nov. 24, 2017, 11:35 a.m. UTC | #2 
On 24/11/17 11:27, Carl Eugen Hoyos wrote:
> 2017-11-24 1:51 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>> This is needed by later hwaccel code to tell which encoding process was
>> used for a particular frame, because hardware decoders may only support a
>> subset of possible methods.
>> ---
>>  libavcodec/avcodec.h  | 6 ++++++
>>  libavcodec/mjpegdec.c | 7 +++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 0972df0bde..c1e68b1d13 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -2897,6 +2897,12 @@ typedef struct AVCodecContext {
>>  #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
>>  #define FF_PROFILE_HEVC_REXT                        4
>>
>> +#define FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT            0xc0
>> +#define FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT 0xc1
>> +#define FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT         0xc2
>> +#define FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS                0xc3
>> +#define FF_PROFILE_MJPEG_JPEG_LS                         0xf7
> 
> Sorry if I misread the code:
> If you (thankfully) define these as in the specification, why don't you
> simply set them without the "if()"'s?

They have to be set inside the switch because there are other non-SOF markers there.

In each case they could then be set from start_code, but it seemed clearer to write the profile out properly?

I can change it to use start_code directly if you prefer.

- Mark
Carl Eugen Hoyos Nov. 24, 2017, 12:05 p.m. UTC | #3 
2017-11-24 12:35 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> On 24/11/17 11:27, Carl Eugen Hoyos wrote:
>> 2017-11-24 1:51 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
>>> This is needed by later hwaccel code to tell which encoding process was
>>> used for a particular frame, because hardware decoders may only support a
>>> subset of possible methods.
>>> ---
>>>  libavcodec/avcodec.h  | 6 ++++++
>>>  libavcodec/mjpegdec.c | 7 +++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index 0972df0bde..c1e68b1d13 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -2897,6 +2897,12 @@ typedef struct AVCodecContext {
>>>  #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
>>>  #define FF_PROFILE_HEVC_REXT                        4
>>>
>>> +#define FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT            0xc0
>>> +#define FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT 0xc1
>>> +#define FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT         0xc2
>>> +#define FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS                0xc3
>>> +#define FF_PROFILE_MJPEG_JPEG_LS                         0xf7
>>
>> Sorry if I misread the code:
>> If you (thankfully) define these as in the specification, why don't you
>> simply set them without the "if()"'s?
>
> They have to be set inside the switch because there are
> other non-SOF markers there.

Of course, thank you!

> In each case they could then be set from start_code, but it
> seemed clearer to write the profile out properly?

See other thread: What is unreadable to one developer
is clearer to another;-)

Carl Eugen
Philip Langdale Nov. 24, 2017, 5 p.m. UTC | #4 
On Fri, 24 Nov 2017 00:51:30 +0000
Mark Thompson <sw@jkqxz.net> wrote:

> This is needed by later hwaccel code to tell which encoding process
> was used for a particular frame, because hardware decoders may only
> support a subset of possible methods.
> ---
>  libavcodec/avcodec.h  | 6 ++++++
>  libavcodec/mjpegdec.c | 7 +++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 0972df0bde..c1e68b1d13 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2897,6 +2897,12 @@ typedef struct AVCodecContext {
>  #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
>  #define FF_PROFILE_HEVC_REXT                        4
>  
> +#define FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT            0xc0
> +#define FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT 0xc1
> +#define FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT         0xc2
> +#define FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS                0xc3
> +#define FF_PROFILE_MJPEG_JPEG_LS                         0xf7
> +
>      /**
>       * level
>       * - encoding: Set by user.
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index e005dd0cd3..f01d44a169 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -2288,6 +2288,10 @@ int ff_mjpeg_decode_frame(AVCodecContext
> *avctx, void *data, int *got_frame, break;
>          case SOF0:
>          case SOF1:
> +            if (start_code == SOF0)
> +                s->avctx->profile =
> FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT;
> +            else
> +                s->avctx->profile =
> FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT; s->lossless    = 0;
>              s->ls          = 0;
>              s->progressive = 0;
> @@ -2295,6 +2299,7 @@ int ff_mjpeg_decode_frame(AVCodecContext
> *avctx, void *data, int *got_frame, goto fail;
>              break;
>          case SOF2:
> +            s->avctx->profile =
> FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT; s->lossless    = 0;
>              s->ls          = 0;
>              s->progressive = 1;
> @@ -2302,6 +2307,7 @@ int ff_mjpeg_decode_frame(AVCodecContext
> *avctx, void *data, int *got_frame, goto fail;
>              break;
>          case SOF3:
> +            s->avctx->profile     =
> FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS; s->avctx->properties |=
> FF_CODEC_PROPERTY_LOSSLESS; s->lossless    = 1;
>              s->ls          = 0;
> @@ -2310,6 +2316,7 @@ int ff_mjpeg_decode_frame(AVCodecContext
> *avctx, void *data, int *got_frame, goto fail;
>              break;
>          case SOF48:
> +            s->avctx->profile     = FF_PROFILE_MJPEG_JPEG_LS;
>              s->avctx->properties |= FF_CODEC_PROPERTY_LOSSLESS;
>              s->lossless    = 1;
>              s->ls          = 1;

LGTM


--phil
Michael Niedermayer Nov. 25, 2017, 1:13 a.m. UTC | #5 
On Fri, Nov 24, 2017 at 12:51:30AM +0000, Mark Thompson wrote:
> This is needed by later hwaccel code to tell which encoding process was
> used for a particular frame, because hardware decoders may only support a
> subset of possible methods.
> ---
>  libavcodec/avcodec.h  | 6 ++++++
>  libavcodec/mjpegdec.c | 7 +++++++
>  2 files changed, 13 insertions(+)

this or a variant of this LGTM

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 0972df0bde..c1e68b1d13 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2897,6 +2897,12 @@  typedef struct AVCodecContext {
 #define FF_PROFILE_HEVC_MAIN_STILL_PICTURE          3
 #define FF_PROFILE_HEVC_REXT                        4
 
+#define FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT            0xc0
+#define FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT 0xc1
+#define FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT         0xc2
+#define FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS                0xc3
+#define FF_PROFILE_MJPEG_JPEG_LS                         0xf7
+
     /**
      * level
      * - encoding: Set by user.
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index e005dd0cd3..f01d44a169 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -2288,6 +2288,10 @@  int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
             break;
         case SOF0:
         case SOF1:
+            if (start_code == SOF0)
+                s->avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_BASELINE_DCT;
+            else
+                s->avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_EXTENDED_SEQUENTIAL_DCT;
             s->lossless    = 0;
             s->ls          = 0;
             s->progressive = 0;
@@ -2295,6 +2299,7 @@  int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
                 goto fail;
             break;
         case SOF2:
+            s->avctx->profile = FF_PROFILE_MJPEG_HUFFMAN_PROGRESSIVE_DCT;
             s->lossless    = 0;
             s->ls          = 0;
             s->progressive = 1;
@@ -2302,6 +2307,7 @@  int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
                 goto fail;
             break;
         case SOF3:
+            s->avctx->profile     = FF_PROFILE_MJPEG_HUFFMAN_LOSSLESS;
             s->avctx->properties |= FF_CODEC_PROPERTY_LOSSLESS;
             s->lossless    = 1;
             s->ls          = 0;
@@ -2310,6 +2316,7 @@  int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
                 goto fail;
             break;
         case SOF48:
+            s->avctx->profile     = FF_PROFILE_MJPEG_JPEG_LS;
             s->avctx->properties |= FF_CODEC_PROPERTY_LOSSLESS;
             s->lossless    = 1;
             s->ls          = 1;