Package level vars - which should be constant - mutated in aacwindowgrouping.go

[cveazey]

In the following block of code:

    case ONLY_LONG_SEQUENCE, LONG_START_SEQUENCE, LONG_STOP_SEQUENCE:
        info.num_windows = 1
        info.num_window_groups = 1
        info.window_group_length = make([]uint8, info.num_window_groups)
        info.window_group_length[info.num_window_groups-1] = 1
        info.num_swb = num_swb_long_windows[idx][sfi]

        info.sect_sfb_offset = append(info.sect_sfb_offset, swb_offset_long_window[sfi][:info.num_swb])
        info.swb_offset = swb_offset_long_window[sfi][:info.num_swb]

        // Special cases for 960's final values so we don't have to duplicate tables
        info.sect_sfb_offset[0] = append(info.sect_sfb_offset[0], framelength)
        info.swb_offset = append(info.swb_offset, framelength)

The line info.sect_sfb_offset[0] = append(info.sect_sfb_offset[0], framelength) ends up mutating some of the underlying swb_offset... slices. This mutation prevents the code from being thread safe, and may have other impacts. I think the root of the issue is

The following playground provides an MVCE of an analogous situation: https://play.golang.org/p/BMcQU5-FS5

I think @jessejlt has an even simpler example.

[cveazey]

Blake, I made this change which I think should solve the issue: https://play.golang.org/p/tXaXpPiCtR

diff --git a/vendor/github.com/Comcast/gaad/aacwindowgrouping.go b/vendor/github.com/Comcast/gaad/aacwindowgrouping.go
index afd884d..fbb212d 100644
--- a/vendor/github.com/Comcast/gaad/aacwindowgrouping.go
+++ b/vendor/github.com/Comcast/gaad/aacwindowgrouping.go
@@ -150,7 +150,9 @@ func window_grouping(info *ics_info, sfi uint8, framelength uint16) {
        info.window_group_length[info.num_window_groups-1] = 1
        info.num_swb = num_swb_long_windows[idx][sfi]

-       info.sect_sfb_offset = append(info.sect_sfb_offset, swb_offset_long_window[sfi][:info.num_swb])
+       t := make([]uint16, len(swb_offset_long_window[sfi][:info.num_swb]), cap(swb_offset_long_window[sfi][:info.num_swb]))
+       copy(t, swb_offset_long_window[sfi][:info.num_swb])
+       info.sect_sfb_offset = append(info.sect_sfb_offset, t)
        info.swb_offset = swb_offset_long_window[sfi][:info.num_swb]

        // Special cases for 960's final values so we don't have to duplicate tables

What do you think?

[BlakeOrth]

Looks good to me as long as it appeases the race detector as well. My only request is that the manual set of cap() is unnecessary since it's automatically set to the length if left unspecified.

Also, in our attempt to expand the relatively poor unit test coverage for this, do you have time to implement a test around the issue?