diff mbox

[FFmpeg-devel,v2,1/2] avcodec/videotoolbox: use AV_WB16 where possible

Message ID 20170927221920.46302-1-ffmpeg@tmm1.net
State New
Headers show

Commit Message

Aman Karmani Sept. 27, 2017, 10:19 p.m. UTC 
From: Aman Gupta <aman@tmm1.net>

additional changes to hevc patchset, as suggested on-list

if these look fine, I will squash into previous patchset and push it to
master.
---
 libavcodec/videotoolbox.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

Comments

James Almer Sept. 27, 2017, 10:45 p.m. UTC | #1 
On 9/27/2017 7:19 PM, Aman Gupta wrote:
> From: Aman Gupta <aman@tmm1.net>
> 
> additional changes to hevc patchset, as suggested on-list
> 
> if these look fine, I will squash into previous patchset and push it to
> master.
> ---
>  libavcodec/videotoolbox.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index 078174cc61..c96200cbdb 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -165,10 +165,8 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
>                   ptlc.non_packed_constraint_flag << 5 |
>                   ptlc.frame_only_constraint_flag << 4);
>      AV_W8(p + 7, 0);
> -    AV_W8(p + 8, 0);
> -    AV_W8(p + 9, 0);
> -    AV_W8(p + 10, 0);
> -    AV_W8(p + 11, 0);
> +    AV_WB16(p + 8, 0);
> +    AV_WB16(p + 10, 0);

AV_WB32(p + 8, 0)

or even AV_WN32(p + 8, 0) since it's 0 and endianness doesn't matter.

>  
>      /* unsigned int(8) general_level_idc; */
>      AV_W8(p + 12, ptlc.level_idc);
> @@ -215,8 +213,7 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
>      AV_W8(p + 18, (sps->bit_depth_chroma - 8) | 0xfc);
>  
>      /* bit(16) avgFrameRate; */
> -    AV_W8(p + 19, 0);
> -    AV_W8(p + 20, 0);
> +    AV_WB16(p + 19, 0);
>  
>      /*
>       * bit(2) constantFrameRate;
> @@ -242,11 +239,9 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
>      AV_W8(p, 1 << 7 |
>               HEVC_NAL_VPS & 0x3f);
>      /* unsigned int(16) numNalus; */
> -    AV_W8(p + 1, 0);
> -    AV_W8(p + 2, 1);
> +    AV_WB16(p + 1, 1);
>      /* unsigned int(16) nalUnitLength; */
> -    AV_W8(p + 3, vps->data_size >> 8);
> -    AV_W8(p + 4, vps->data_size & 0xffff);
> +    AV_WB16(p + 3, vps->data_size);
>      /* bit(8*nalUnitLength) nalUnit; */
>      memcpy(p + 5, vps->data, vps->data_size);
>      p += 5 + vps->data_size;
> @@ -254,24 +249,20 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
>      /* sps */
>      AV_W8(p, 1 << 7 |
>               HEVC_NAL_SPS & 0x3f);
> -    AV_W8(p + 1, 0);
> -    AV_W8(p + 2, 1);
> -    AV_W8(p + 3, sps->data_size >> 8);
> -    AV_W8(p + 4, sps->data_size & 0xffff);
> +    AV_WB16(p + 1, 1);
> +    AV_WB16(p + 3, sps->data_size);
>      memcpy(p + 5, sps->data, sps->data_size);
>      p += 5 + sps->data_size;
>  
>      /* pps */
>      AV_W8(p, 1 << 7 |
>               HEVC_NAL_PPS & 0x3f);
> -    AV_W8(p + 1, num_pps >> 8);
> -    AV_W8(p + 2, num_pps & 0xffff);
> +    AV_WB16(p + 1, num_pps);
>      p += 3;
>      for (i = 0; i < MAX_PPS_COUNT; i++) {
>          if (h->ps.pps_list[i]) {
>              const HEVCPPS *pps = (const HEVCPPS *)h->ps.pps_list[i]->data;
> -            AV_W8(p + 0, pps->data_size >> 8);
> -            AV_W8(p + 1, pps->data_size & 0xffff);
> +            AV_WB16(p, pps->data_size);
>              memcpy(p + 2, pps->data, pps->data_size);
>              p += 2 + pps->data_size;
>          }
>
wm4 Sept. 27, 2017, 11:13 p.m. UTC | #2 
On Wed, 27 Sep 2017 19:45:23 -0300
James Almer <jamrial@gmail.com> wrote:

> On 9/27/2017 7:19 PM, Aman Gupta wrote:
> > From: Aman Gupta <aman@tmm1.net>
> > 
> > additional changes to hevc patchset, as suggested on-list
> > 
> > if these look fine, I will squash into previous patchset and push it to
> > master.
> > ---
> >  libavcodec/videotoolbox.c | 27 +++++++++------------------
> >  1 file changed, 9 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> > index 078174cc61..c96200cbdb 100644
> > --- a/libavcodec/videotoolbox.c
> > +++ b/libavcodec/videotoolbox.c
> > @@ -165,10 +165,8 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
> >                   ptlc.non_packed_constraint_flag << 5 |
> >                   ptlc.frame_only_constraint_flag << 4);
> >      AV_W8(p + 7, 0);
> > -    AV_W8(p + 8, 0);
> > -    AV_W8(p + 9, 0);
> > -    AV_W8(p + 10, 0);
> > -    AV_W8(p + 11, 0);
> > +    AV_WB16(p + 8, 0);
> > +    AV_WB16(p + 10, 0);  
> 
> AV_WB32(p + 8, 0)
> 
> or even AV_WN32(p + 8, 0) since it's 0 and endianness doesn't matter.

IMO it should reflect whatever bitstream elements these are. Ideally,
we'd have comments about what each element is too.
Aman Karmani Sept. 27, 2017, 11:22 p.m. UTC | #3 
On Wed, Sep 27, 2017 at 4:13 PM, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed, 27 Sep 2017 19:45:23 -0300
> James Almer <jamrial@gmail.com> wrote:
>
> > On 9/27/2017 7:19 PM, Aman Gupta wrote:
> > > From: Aman Gupta <aman@tmm1.net>
> > >
> > > additional changes to hevc patchset, as suggested on-list
> > >
> > > if these look fine, I will squash into previous patchset and push it to
> > > master.
> > > ---
> > >  libavcodec/videotoolbox.c | 27 +++++++++------------------
> > >  1 file changed, 9 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> > > index 078174cc61..c96200cbdb 100644
> > > --- a/libavcodec/videotoolbox.c
> > > +++ b/libavcodec/videotoolbox.c
> > > @@ -165,10 +165,8 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext
> *avctx)
> > >                   ptlc.non_packed_constraint_flag << 5 |
> > >                   ptlc.frame_only_constraint_flag << 4);
> > >      AV_W8(p + 7, 0);
> > > -    AV_W8(p + 8, 0);
> > > -    AV_W8(p + 9, 0);
> > > -    AV_W8(p + 10, 0);
> > > -    AV_W8(p + 11, 0);
> > > +    AV_WB16(p + 8, 0);
> > > +    AV_WB16(p + 10, 0);
> >
> > AV_WB32(p + 8, 0)
> >
> > or even AV_WN32(p + 8, 0) since it's 0 and endianness doesn't matter.
>
> IMO it should reflect whatever bitstream elements these are. Ideally,
> we'd have comments about what each element is too.
>

There is already a comment in this section. It's supposed to be 48bits of
flags, but the spec has 44 of those bits defined as 'reserved zero bits'.

    /* unsigned int(48) general_constraint_indicator_flags; */
    AV_W8(p + 6, ptlc.progressive_source_flag    << 7 |
                   ptlc.interlaced_source_flag     << 6 |
                   ptlc.non_packed_constraint_flag << 5 |
                   ptlc.frame_only_constraint_flag << 4);
    AV_W8(p + 7, 0);
    AV_WN32(p + 8, 0);

In hvcc_write (lavf/hevc.c), libavformat uses:

    /* unsigned int(48) general_constraint_indicator_flags; */
    avio_wb32(pb, hvcc->general_constraint_indicator_flags >> 16);
    avio_wb16(pb, hvcc->general_constraint_indicator_flags);

But the PTL parsing code in libavcodec defines the flags individually.
See decode_profile_tier_level (lavc/hevc_ps.c):

    ptl->progressive_source_flag    = get_bits1(gb);
    ptl->interlaced_source_flag     = get_bits1(gb);
    ptl->non_packed_constraint_flag = get_bits1(gb);
    ptl->frame_only_constraint_flag = get_bits1(gb);

    skip_bits(gb, 16); // XXX_reserved_zero_44bits[0..15]
    skip_bits(gb, 16); // XXX_reserved_zero_44bits[16..31]
    skip_bits(gb, 12); // XXX_reserved_zero_44bits[32..43]

Aman


> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
index 078174cc61..c96200cbdb 100644
--- a/libavcodec/videotoolbox.c
+++ b/libavcodec/videotoolbox.c
@@ -165,10 +165,8 @@  CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
                  ptlc.non_packed_constraint_flag << 5 |
                  ptlc.frame_only_constraint_flag << 4);
     AV_W8(p + 7, 0);
-    AV_W8(p + 8, 0);
-    AV_W8(p + 9, 0);
-    AV_W8(p + 10, 0);
-    AV_W8(p + 11, 0);
+    AV_WB16(p + 8, 0);
+    AV_WB16(p + 10, 0);
 
     /* unsigned int(8) general_level_idc; */
     AV_W8(p + 12, ptlc.level_idc);
@@ -215,8 +213,7 @@  CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
     AV_W8(p + 18, (sps->bit_depth_chroma - 8) | 0xfc);
 
     /* bit(16) avgFrameRate; */
-    AV_W8(p + 19, 0);
-    AV_W8(p + 20, 0);
+    AV_WB16(p + 19, 0);
 
     /*
      * bit(2) constantFrameRate;
@@ -242,11 +239,9 @@  CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
     AV_W8(p, 1 << 7 |
              HEVC_NAL_VPS & 0x3f);
     /* unsigned int(16) numNalus; */
-    AV_W8(p + 1, 0);
-    AV_W8(p + 2, 1);
+    AV_WB16(p + 1, 1);
     /* unsigned int(16) nalUnitLength; */
-    AV_W8(p + 3, vps->data_size >> 8);
-    AV_W8(p + 4, vps->data_size & 0xffff);
+    AV_WB16(p + 3, vps->data_size);
     /* bit(8*nalUnitLength) nalUnit; */
     memcpy(p + 5, vps->data, vps->data_size);
     p += 5 + vps->data_size;
@@ -254,24 +249,20 @@  CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext *avctx)
     /* sps */
     AV_W8(p, 1 << 7 |
              HEVC_NAL_SPS & 0x3f);
-    AV_W8(p + 1, 0);
-    AV_W8(p + 2, 1);
-    AV_W8(p + 3, sps->data_size >> 8);
-    AV_W8(p + 4, sps->data_size & 0xffff);
+    AV_WB16(p + 1, 1);
+    AV_WB16(p + 3, sps->data_size);
     memcpy(p + 5, sps->data, sps->data_size);
     p += 5 + sps->data_size;
 
     /* pps */
     AV_W8(p, 1 << 7 |
              HEVC_NAL_PPS & 0x3f);
-    AV_W8(p + 1, num_pps >> 8);
-    AV_W8(p + 2, num_pps & 0xffff);
+    AV_WB16(p + 1, num_pps);
     p += 3;
     for (i = 0; i < MAX_PPS_COUNT; i++) {
         if (h->ps.pps_list[i]) {
             const HEVCPPS *pps = (const HEVCPPS *)h->ps.pps_list[i]->data;
-            AV_W8(p + 0, pps->data_size >> 8);
-            AV_W8(p + 1, pps->data_size & 0xffff);
+            AV_WB16(p, pps->data_size);
             memcpy(p + 2, pps->data, pps->data_size);
             p += 2 + pps->data_size;
         }