[1/2] hevcdec: remove HEVCContext usage from hevc_sei

Message ID 20170430153502.7720-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 30, 2017, 3:35 p.m.
Based on the H264 SEI implementation.

This will be mainly useful once support for SEI messages that can be
used by the hevc parser are implemented, like Picture Timing.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/hevc_sei.c | 77 +++++++++++++++++++++++++--------------------------
 libavcodec/hevcdec.c  | 49 +++++++++++++++++---------------
 libavcodec/hevcdec.h  | 44 ++++++++++++++++++-----------
 3 files changed, 92 insertions(+), 78 deletions(-)

Comments

James Almer May 5, 2017, 6:56 p.m. | #1
On 4/30/2017 12:35 PM, James Almer wrote:
> Based on the H264 SEI implementation.
> 
> This will be mainly useful once support for SEI messages that can be
> used by the hevc parser are implemented, like Picture Timing.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/hevc_sei.c | 77 +++++++++++++++++++++++++--------------------------
>  libavcodec/hevcdec.c  | 49 +++++++++++++++++---------------
>  libavcodec/hevcdec.h  | 44 ++++++++++++++++++-----------
>  3 files changed, 92 insertions(+), 78 deletions(-)

Ping for set.

> 
> diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
> index 1f8554ad5..bb4980085 100644
> --- a/libavcodec/hevc_sei.c
> +++ b/libavcodec/hevc_sei.c
> @@ -53,10 +53,10 @@ enum HEVC_SEI_TYPE {
>      SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO             = 144,
>  };
>  
> -static int decode_nal_sei_decoded_picture_hash(HEVCContext *s)
> +static int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s, GetBitContext *gb,
> +                                               void *logctx)

Btw, I could send a new version removing the logctx parameter, since i
noticed after sending that it's unused in a bunch of these functions.
Luca Barbato May 5, 2017, 8:32 p.m. | #2
On 5/5/17 8:56 PM, James Almer wrote:
> On 4/30/2017 12:35 PM, James Almer wrote:
>> Based on the H264 SEI implementation.
>>
>> This will be mainly useful once support for SEI messages that can be
>> used by the hevc parser are implemented, like Picture Timing.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/hevc_sei.c | 77 +++++++++++++++++++++++++--------------------------
>>  libavcodec/hevcdec.c  | 49 +++++++++++++++++---------------
>>  libavcodec/hevcdec.h  | 44 ++++++++++++++++++-----------
>>  3 files changed, 92 insertions(+), 78 deletions(-)
> 
> Ping for set.

HEVCSEIContext doesn't seem a context though, probably HEVCSEIInfo might
be a better name.

Beside that seems fine to me.

> Btw, I could send a new version removing the logctx parameter, since i
> noticed after sending that it's unused in a bunch of these functions.

Sounds a good idea.

lu
Hendrik Leppkes May 5, 2017, 8:52 p.m. | #3
On Fri, May 5, 2017 at 10:32 PM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 5/5/17 8:56 PM, James Almer wrote:
>> On 4/30/2017 12:35 PM, James Almer wrote:
>>> Based on the H264 SEI implementation.
>>>
>>> This will be mainly useful once support for SEI messages that can be
>>> used by the hevc parser are implemented, like Picture Timing.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavcodec/hevc_sei.c | 77 +++++++++++++++++++++++++--------------------------
>>>  libavcodec/hevcdec.c  | 49 +++++++++++++++++---------------
>>>  libavcodec/hevcdec.h  | 44 ++++++++++++++++++-----------
>>>  3 files changed, 92 insertions(+), 78 deletions(-)
>>
>> Ping for set.
>
> HEVCSEIContext doesn't seem a context though, probably HEVCSEIInfo might
> be a better name.
>
> Beside that seems fine to me.
>

The name is consistent with the H264 decoder struct naming, which is
called H264SEIContext, so imho its fine.

- Hendrik
James Almer May 7, 2017, 3:29 a.m. | #4
On 5/5/2017 5:32 PM, Luca Barbato wrote:
> On 5/5/17 8:56 PM, James Almer wrote:
>> On 4/30/2017 12:35 PM, James Almer wrote:
>>> Based on the H264 SEI implementation.
>>>
>>> This will be mainly useful once support for SEI messages that can be
>>> used by the hevc parser are implemented, like Picture Timing.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  libavcodec/hevc_sei.c | 77 +++++++++++++++++++++++++--------------------------
>>>  libavcodec/hevcdec.c  | 49 +++++++++++++++++---------------
>>>  libavcodec/hevcdec.h  | 44 ++++++++++++++++++-----------
>>>  3 files changed, 92 insertions(+), 78 deletions(-)
>>
>> Ping for set.
> 
> HEVCSEIContext doesn't seem a context though, probably HEVCSEIInfo might
> be a better name.

How about ATMMachine or PINNumber? :P

Jokes aside, I personally agree with Hendrik. HEVCSEIContext is
consistent with h264's naming. But if you insist i can change it to for
example HEVCSEI, following the existing HEVCSPS/HEVCPPS/HEVCVPS structs
for the corresponding NALUs.

> 
> Beside that seems fine to me.
> 
>> Btw, I could send a new version removing the logctx parameter, since i
>> noticed after sending that it's unused in a bunch of these functions.
> 
> Sounds a good idea.
> 
> lu
> _______________________________________________
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>
Luca Barbato May 7, 2017, 12:26 p.m. | #5
On 5/7/17 5:29 AM, James Almer wrote:
> Jokes aside, I personally agree with Hendrik. HEVCSEIContext is
> consistent with h264's naming. But if you insist i can change it to for
> example HEVCSEI, following the existing HEVCSPS/HEVCPPS/HEVCVPS structs
> for the corresponding NALUs.

Would be nice, thank you.

lu

Patch

diff --git a/libavcodec/hevc_sei.c b/libavcodec/hevc_sei.c
index 1f8554ad5..bb4980085 100644
--- a/libavcodec/hevc_sei.c
+++ b/libavcodec/hevc_sei.c
@@ -53,10 +53,10 @@  enum HEVC_SEI_TYPE {
     SEI_TYPE_CONTENT_LIGHT_LEVEL_INFO             = 144,
 };
 
-static int decode_nal_sei_decoded_picture_hash(HEVCContext *s)
+static int decode_nal_sei_decoded_picture_hash(HEVCSEIPictureHash *s, GetBitContext *gb,
+                                               void *logctx)
 {
     int cIdx, i;
-    GetBitContext *gb = &s->HEVClc.gb;
     uint8_t hash_type = get_bits(gb, 8);
 
     for (cIdx = 0; cIdx < 3; cIdx++) {
@@ -75,15 +75,15 @@  static int decode_nal_sei_decoded_picture_hash(HEVCContext *s)
     return 0;
 }
 
-static int decode_nal_sei_frame_packing_arrangement(HEVCContext *s)
+static int decode_nal_sei_frame_packing_arrangement(HEVCSEIFramePacking *s,
+                                                    GetBitContext *gb,
+                                                    void *logctx)
 {
-    GetBitContext *gb = &s->HEVClc.gb;
-
     get_ue_golomb(gb);                  // frame_packing_arrangement_id
-    s->sei_frame_packing_present = !get_bits1(gb);
+    s->present = !get_bits1(gb);
 
-    if (s->sei_frame_packing_present) {
-        s->frame_packing_arrangement_type = get_bits(gb, 7);
+    if (s->present) {
+        s->arrangement_type               = get_bits(gb, 7);
         s->quincunx_subsampling           = get_bits1(gb);
         s->content_interpretation_type    = get_bits(gb, 6);
 
@@ -92,7 +92,7 @@  static int decode_nal_sei_frame_packing_arrangement(HEVCContext *s)
         // frame0_self_contained_flag frame1_self_contained_flag
         skip_bits(gb, 6);
 
-        if (!s->quincunx_subsampling && s->frame_packing_arrangement_type != 5)
+        if (!s->quincunx_subsampling && s->arrangement_type != 5)
             skip_bits(gb, 16);  // frame[01]_grid_position_[xy]
         skip_bits(gb, 8);       // frame_packing_arrangement_reserved_byte
         skip_bits1(gb);         // frame_packing_arrangement_persistence_flag
@@ -101,63 +101,61 @@  static int decode_nal_sei_frame_packing_arrangement(HEVCContext *s)
     return 0;
 }
 
-static int decode_nal_sei_display_orientation(HEVCContext *s)
+static int decode_nal_sei_display_orientation(HEVCSEIDisplayOrientation *s, GetBitContext *gb,
+                                              void *logctx)
 {
-    GetBitContext *gb = &s->HEVClc.gb;
-
-    s->sei_display_orientation_present = !get_bits1(gb);
+    s->present = !get_bits1(gb);
 
-    if (s->sei_display_orientation_present) {
-        s->sei_hflip = get_bits1(gb);     // hor_flip
-        s->sei_vflip = get_bits1(gb);     // ver_flip
+    if (s->present) {
+        s->hflip = get_bits1(gb);     // hor_flip
+        s->vflip = get_bits1(gb);     // ver_flip
 
-        s->sei_anticlockwise_rotation = get_bits(gb, 16);
+        s->anticlockwise_rotation = get_bits(gb, 16);
         skip_bits1(gb);     // display_orientation_persistence_flag
     }
 
     return 0;
 }
 
-static int decode_nal_sei_prefix(HEVCContext *s, int type, int size)
+static int decode_nal_sei_prefix(GetBitContext *gb, void *logctx, HEVCSEIContext *s,
+                                 int type, int size)
 {
-    GetBitContext *gb = &s->HEVClc.gb;
-
     switch (type) {
     case 256:  // Mismatched value from HM 8.1
-        return decode_nal_sei_decoded_picture_hash(s);
+        return decode_nal_sei_decoded_picture_hash(&s->picture_hash, gb, logctx);
     case SEI_TYPE_FRAME_PACKING:
-        return decode_nal_sei_frame_packing_arrangement(s);
+        return decode_nal_sei_frame_packing_arrangement(&s->frame_packing,
+                                                        gb, logctx);
     case SEI_TYPE_DISPLAY_ORIENTATION:
-        return decode_nal_sei_display_orientation(s);
+        return decode_nal_sei_display_orientation(&s->display_orientation,
+                                                  gb, logctx);
     default:
-        av_log(s->avctx, AV_LOG_DEBUG, "Skipped PREFIX SEI %d\n", type);
+        av_log(logctx, AV_LOG_DEBUG, "Skipped PREFIX SEI %d\n", type);
         skip_bits_long(gb, 8 * size);
         return 0;
     }
 }
 
-static int decode_nal_sei_suffix(HEVCContext *s, int type, int size)
+static int decode_nal_sei_suffix(GetBitContext *gb, void *logctx, HEVCSEIContext *s,
+                                 int type, int size)
 {
-    GetBitContext *gb = &s->HEVClc.gb;
-
     switch (type) {
     case SEI_TYPE_DECODED_PICTURE_HASH:
-        return decode_nal_sei_decoded_picture_hash(s);
+        return decode_nal_sei_decoded_picture_hash(&s->picture_hash, gb, logctx);
     default:
-        av_log(s->avctx, AV_LOG_DEBUG, "Skipped SUFFIX SEI %d\n", type);
+        av_log(logctx, AV_LOG_DEBUG, "Skipped SUFFIX SEI %d\n", type);
         skip_bits_long(gb, 8 * size);
         return 0;
     }
 }
 
-static int decode_nal_sei_message(HEVCContext *s)
+static int decode_nal_sei_message(GetBitContext *gb, void *logctx, HEVCSEIContext *s,
+                                  int type)
 {
-    GetBitContext *gb = &s->HEVClc.gb;
-
     int payload_type = 0;
     int payload_size = 0;
     int byte = 0xFF;
-    av_log(s->avctx, AV_LOG_DEBUG, "Decoding SEI\n");
+    av_log(logctx, AV_LOG_DEBUG, "Decoding SEI\n");
 
     while (byte == 0xFF) {
         byte          = get_bits(gb, 8);
@@ -168,10 +166,10 @@  static int decode_nal_sei_message(HEVCContext *s)
         byte          = get_bits(gb, 8);
         payload_size += byte;
     }
-    if (s->nal_unit_type == HEVC_NAL_SEI_PREFIX) {
-        return decode_nal_sei_prefix(s, payload_type, payload_size);
+    if (type == HEVC_NAL_SEI_PREFIX) {
+        return decode_nal_sei_prefix(gb, logctx, s, payload_type, payload_size);
     } else { /* nal_unit_type == NAL_SEI_SUFFIX */
-        return decode_nal_sei_suffix(s, payload_type, payload_size);
+        return decode_nal_sei_suffix(gb, logctx, s, payload_type, payload_size);
     }
 }
 
@@ -180,10 +178,11 @@  static int more_rbsp_data(GetBitContext *gb)
     return get_bits_left(gb) > 0 && show_bits(gb, 8) != 0x80;
 }
 
-int ff_hevc_decode_nal_sei(HEVCContext *s)
+int ff_hevc_decode_nal_sei(GetBitContext *gb, void *logctx, HEVCSEIContext *s,
+                           int type)
 {
     do {
-        decode_nal_sei_message(s);
-    } while (more_rbsp_data(&s->HEVClc.gb));
+        decode_nal_sei_message(gb, logctx, s, type);
+    } while (more_rbsp_data(gb));
     return 0;
 }
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 6a0485858..96e47cd18 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -2362,18 +2362,18 @@  static int set_side_data(HEVCContext *s)
 {
     AVFrame *out = s->ref->frame;
 
-    if (s->sei_frame_packing_present &&
-        s->frame_packing_arrangement_type >= 3 &&
-        s->frame_packing_arrangement_type <= 5 &&
-        s->content_interpretation_type > 0 &&
-        s->content_interpretation_type < 3) {
+    if (s->sei.frame_packing.present &&
+        s->sei.frame_packing.arrangement_type >= 3 &&
+        s->sei.frame_packing.arrangement_type <= 5 &&
+        s->sei.frame_packing.content_interpretation_type > 0 &&
+        s->sei.frame_packing.content_interpretation_type < 3) {
         AVStereo3D *stereo = av_stereo3d_create_side_data(out);
         if (!stereo)
             return AVERROR(ENOMEM);
 
-        switch (s->frame_packing_arrangement_type) {
+        switch (s->sei.frame_packing.arrangement_type) {
         case 3:
-            if (s->quincunx_subsampling)
+            if (s->sei.frame_packing.quincunx_subsampling)
                 stereo->type = AV_STEREO3D_SIDEBYSIDE_QUINCUNX;
             else
                 stereo->type = AV_STEREO3D_SIDEBYSIDE;
@@ -2386,13 +2386,14 @@  static int set_side_data(HEVCContext *s)
             break;
         }
 
-        if (s->content_interpretation_type == 2)
+        if (s->sei.frame_packing.content_interpretation_type == 2)
             stereo->flags = AV_STEREO3D_FLAG_INVERT;
     }
 
-    if (s->sei_display_orientation_present &&
-        (s->sei_anticlockwise_rotation || s->sei_hflip || s->sei_vflip)) {
-        double angle = s->sei_anticlockwise_rotation * 360 / (double) (1 << 16);
+    if (s->sei.display_orientation.present &&
+        (s->sei.display_orientation.anticlockwise_rotation ||
+         s->sei.display_orientation.hflip || s->sei.display_orientation.vflip)) {
+        double angle = s->sei.display_orientation.anticlockwise_rotation * 360 / (double) (1 << 16);
         AVFrameSideData *rotation = av_frame_new_side_data(out,
                                                            AV_FRAME_DATA_DISPLAYMATRIX,
                                                            sizeof(int32_t) * 9);
@@ -2401,7 +2402,8 @@  static int set_side_data(HEVCContext *s)
 
         av_display_rotation_set((int32_t *)rotation->data, angle);
         av_display_matrix_flip((int32_t *)rotation->data,
-                               s->sei_hflip, s->sei_vflip);
+                               s->sei.display_orientation.hflip,
+                               s->sei.display_orientation.vflip);
     }
 
     return 0;
@@ -2486,7 +2488,8 @@  static int decode_nal_unit(HEVCContext *s, const H2645NAL *nal)
         break;
     case HEVC_NAL_SEI_PREFIX:
     case HEVC_NAL_SEI_SUFFIX:
-        ret = ff_hevc_decode_nal_sei(s);
+        ret = ff_hevc_decode_nal_sei(gb, s->avctx, &s->sei,
+                                     s->nal_unit_type);
         if (ret < 0)
             goto fail;
         break;
@@ -2680,7 +2683,7 @@  static int verify_md5(HEVCContext *s, AVFrame *frame)
         int h = (i == 1 || i == 2) ? (height >> desc->log2_chroma_h) : height;
         uint8_t md5[16];
 
-        av_md5_init(s->md5_ctx);
+        av_md5_init(s->sei.picture_hash.md5_ctx);
         for (j = 0; j < h; j++) {
             const uint8_t *src = frame->data[i] + j * frame->linesize[i];
 #if HAVE_BIGENDIAN
@@ -2690,11 +2693,11 @@  static int verify_md5(HEVCContext *s, AVFrame *frame)
                 src = s->checksum_buf;
             }
 #endif
-            av_md5_update(s->md5_ctx, src, w << pixel_shift);
+            av_md5_update(s->sei.picture_hash.md5_ctx, src, w << pixel_shift);
         }
-        av_md5_final(s->md5_ctx, md5);
+        av_md5_final(s->sei.picture_hash.md5_ctx, md5);
 
-        if (!memcmp(md5, s->md5[i], 16)) {
+        if (!memcmp(md5, s->sei.picture_hash.md5[i], 16)) {
             av_log   (s->avctx, AV_LOG_DEBUG, "plane %d - correct ", i);
             print_md5(s->avctx, AV_LOG_DEBUG, md5);
             av_log   (s->avctx, AV_LOG_DEBUG, "; ");
@@ -2702,7 +2705,7 @@  static int verify_md5(HEVCContext *s, AVFrame *frame)
             av_log   (s->avctx, AV_LOG_ERROR, "mismatching checksum of plane %d - ", i);
             print_md5(s->avctx, AV_LOG_ERROR, md5);
             av_log   (s->avctx, AV_LOG_ERROR, " != ");
-            print_md5(s->avctx, AV_LOG_ERROR, s->md5[i]);
+            print_md5(s->avctx, AV_LOG_ERROR, s->sei.picture_hash.md5[i]);
             av_log   (s->avctx, AV_LOG_ERROR, "\n");
             return AVERROR_INVALIDDATA;
         }
@@ -2822,7 +2825,7 @@  static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output,
     } else {
         /* verify the SEI checksum */
         if (avctx->err_recognition & AV_EF_CRCCHECK && s->is_decoded &&
-            s->is_md5) {
+            s->sei.picture_hash.is_md5) {
             ret = verify_md5(s, s->ref->frame);
             if (ret < 0 && avctx->err_recognition & AV_EF_EXPLODE) {
                 ff_hevc_unref_frame(s, s->ref, ~0);
@@ -2830,7 +2833,7 @@  static int hevc_decode_frame(AVCodecContext *avctx, void *data, int *got_output,
             }
         }
     }
-    s->is_md5 = 0;
+    s->sei.picture_hash.is_md5 = 0;
 
     if (s->is_decoded) {
         av_log(avctx, AV_LOG_DEBUG, "Decoded frame with POC %d.\n", s->poc);
@@ -2890,7 +2893,7 @@  static av_cold int hevc_decode_free(AVCodecContext *avctx)
 
     pic_arrays_free(s);
 
-    av_freep(&s->md5_ctx);
+    av_freep(&s->sei.picture_hash.md5_ctx);
 
     av_frame_free(&s->tmp_frame);
     av_frame_free(&s->output_frame);
@@ -2936,8 +2939,8 @@  static av_cold int hevc_init_context(AVCodecContext *avctx)
 
     s->max_ra = INT_MAX;
 
-    s->md5_ctx = av_md5_alloc();
-    if (!s->md5_ctx)
+    s->sei.picture_hash.md5_ctx = av_md5_alloc();
+    if (!s->sei.picture_hash.md5_ctx)
         goto fail;
 
     ff_bswapdsp_init(&s->bdsp);
diff --git a/libavcodec/hevcdec.h b/libavcodec/hevcdec.h
index ff192f67a..9526a8838 100644
--- a/libavcodec/hevcdec.h
+++ b/libavcodec/hevcdec.h
@@ -444,6 +444,31 @@  typedef struct HEVCLocalContext {
     int boundary_flags;
 } HEVCLocalContext;
 
+typedef struct HEVCSEIPictureHash {
+    struct AVMD5 *md5_ctx;
+    uint8_t       md5[3][16];
+    uint8_t is_md5;
+} HEVCSEIPictureHash;
+
+typedef struct HEVCSEIFramePacking {
+    int present;
+    int arrangement_type;
+    int content_interpretation_type;
+    int quincunx_subsampling;
+} HEVCSEIFramePacking;
+
+typedef struct HEVCSEIDisplayOrientation {
+    int present;
+    int anticlockwise_rotation;
+    int hflip, vflip;
+} HEVCSEIDisplayOrientation;
+
+typedef struct HEVCSEIContext {
+    HEVCSEIPictureHash picture_hash;
+    HEVCSEIFramePacking frame_packing;
+    HEVCSEIDisplayOrientation display_orientation;
+} HEVCSEIContext;
+
 typedef struct HEVCContext {
     const AVClass *c;  // needed by private avoptions
     AVCodecContext *avctx;
@@ -522,11 +547,6 @@  typedef struct HEVCContext {
     // type of the first VCL NAL of the current frame
     enum HEVCNALUnitType first_nal_type;
 
-    // for checking the frame checksums
-    struct AVMD5 *md5_ctx;
-    uint8_t       md5[3][16];
-    uint8_t is_md5;
-
     uint8_t context_initialized;
     uint8_t is_nalff;       ///< this flag is != 0 if bitstream is encapsulated
                             ///< as a format defined in 14496-15
@@ -535,19 +555,11 @@  typedef struct HEVCContext {
     int nal_length_size;    ///< Number of bytes used for nal length (1, 2 or 4)
     int nuh_layer_id;
 
-    /** frame packing arrangement variables */
-    int sei_frame_packing_present;
-    int frame_packing_arrangement_type;
-    int content_interpretation_type;
-    int quincunx_subsampling;
-
-    /** display orientation */
-    int sei_display_orientation_present;
-    int sei_anticlockwise_rotation;
-    int sei_hflip, sei_vflip;
+    HEVCSEIContext sei;
 } HEVCContext;
 
-int ff_hevc_decode_nal_sei(HEVCContext *s);
+int ff_hevc_decode_nal_sei(GetBitContext *gb, void *logctx, HEVCSEIContext *s,
+                           int type);
 
 /**
  * Mark all frames in DPB as unused for reference.