o.downcast crash

[ramagottfried]

o.downcast crashes for me when receiving a nested subbundle -- maybe it should automatically flatten the bundle?

----------begin_max5_patcher----------
574.3oc0UtsiZCCD.84vWQjeNkZ6bC5GPetuWVgLICKdavNJ1gksH92qujv0
PWPc0JUoHmL2rGO93I6FEfVH2BJT32B+YXPvtQAANUVEAcxAn0rsEULkyMjb
bgbcsTAnHuUMrU6r7Ukl0nMeQ5MsTJzKYEf0L9DcJ9uc5Hzw8pqY5hUbwyya
fBsOeHSyLlCiISruHoX6KZxXb3ScAwK8ozhW9Rb+ZJa0UfV+VM3mED5f60Mf
BDZllKE2bcvcCGhxt+JjUxFu23wjISht3E4D+Esq4BSJ3JWziJ8IlSKoSqhs
AJmunUTVAyq.wy5UVywYCYujoY9THNMJbpcUI41AawA6Fr5vDSVcGOjrnvDW
7o1AizTmTRmj2qDqHN85nCexlh6GMxND8uyNZ9Zv7wtYhY5ifzLw9dWq3BnP
1Jb9G+w.XSxcjUL4D.KkLLfQuC.6yGUxoeBnRJsGUx5nABd5kwaIkjrqh0vx
++xqB3UyQ+Uvpb7xJlVCBzsYq3be6JL0AUwNA5vnE4ln02aqp9Aq3Wf9c6hQ
8cwn3jA6hcFpc.pzuU4VJDZX36irlUJeUTvTZz6dgjF65DmS9aEMZ5CU0F7p
1ir+cyjqKzE+tzsOr5OunnjsME8YTWCjvi6jRPo4B2g3o9jdlSq3kkf3zlak
bEaQE3JC3AOct6zI8NxGyeUsWcd3zwWrX00afFU2T5xDCq7hu8XdjSjK7htN
5nFXCu2euFViARzFBosweRsMKA4CUVBMhVd2AkYk2O5OX6k9PC
-----------end_max5_patcher-----------

[ramagottfried]

fixed by re adding the flattening routine (it was there but commented out).

Flattening does probably add some overhead if you were sending a lot of data through it might be noticeable, however I think it's probably necessary.

The previous version attempted to copied each sub bundle into the output bundle, which was the part where it was crashing.

Rather than fix this part, I re-added the flattening, since copying the sub-bundle doesn't deal with sub-sub-bundles etc.

[maccallum]

What was o.downcast doing with sub bundles with that code commented out? Flattening is definitely not the right thing to do—if you want that behavior, stick an o.flatten above o.downcast.

On Oct 6, 2017, at 12:39 PM, rama gottfried notifications@github.com wrote:

fixed by re adding the flattening routine (it was there but commented out).

Flattening does probably add some overhead if you were sending a lot of data through it might be noticeable, however I think it's probably necessary.

The previous version attempted to copied each sub bundle into the output bundle, which was the part where it was crashing.

Rather than fix this part, I re-added the flattening, since copying the sub-bundle doesn't deal with sub-sub-bundles etc.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334720471, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdZhDpqUOm1ci3mxlDDpNOwbk8jJ5ks5spgNMgaJpZM4OFFX3.

[ramagottfried]

hey! I was hoping you might see this. what should o.downcast do with subbundles then?

the thing that I commented out (which I think was maybe indirectly causing the crash) looks like it was attempting to copy the subbundles into the output bundle:

for(int i = 0; i < nnestedbundles; i++){
    t_osc_bndl_s *bs2 = osc_bundle_u_serialize(nestedbundles[i]);
    if(bs2){
        long ll = osc_bundle_s_getLen(bs2);
        char *pp = osc_bundle_s_getPtr(bs2);
        p = osc_mem_resize(p, l + ll);
        memcpy(p + l, pp, ll);
        l += ll;
        osc_bundle_s_deepFree(bs2);
    }
}

[ramagottfried]

the crash was happening with the deep free after outputting the bundle (with bs1)

[ramagottfried]

ok, so I just removed the flattening, and was able to keep it from crashing by commenting out just the subbundle memcpy part in the if(bs2){} block. (see below)

The result is that the address comes out with no values attached to it, which seems reasonable -- or maybe it should say something like: /foo : "subbundle" or something like that?

        if(bs1){
        long l = osc_bundle_s_getLen(bs1);
        char *p = osc_bundle_s_getPtr(bs1);

        memcpy(p + OSC_ID_SIZE, &timetag, sizeof(t_osc_timetag));

        for(int i = 0; i < nnestedbundles; i++){
            t_osc_bndl_s *bs2 = osc_bundle_u_serialize(nestedbundles[i]);
            if(bs2){
                       /*       
                               long ll = osc_bundle_s_getLen(bs2);
                char *pp = osc_bundle_s_getPtr(bs2);
                p = osc_mem_resize(p, l + ll);
                memcpy(p + l, pp, ll);
                l += ll;
                                */
                osc_bundle_s_deepFree(bs2);
            }
        }

        //if(x->bundle){
        omax_util_outletOSC(x->outlet, l, p);
            /*
        }else{
            t_osc_bndl_it_s *bit = osc_bndl_it_s_get(l, p);
            while(osc_bndl_it_s_hasNext(bit)){
                t_osc_msg_s *m = osc_bndl_it_s_next(bit);
                long ml = osc_message_s_getSize(m);
                char *mp = osc_message_s_getAddress(m);
                omax_util_outletOSC(x->outlet, ml, mp);
            }
            osc_bndl_it_s_destroy(bit);
        }
            */
        osc_bundle_s_deepFree(bs1);
    }

[maccallum]

I think the only sane thing is to turn sub bundles into blobs—if they’re serialized already, that just involves changing the type tag.

On Oct 6, 2017, at 4:33 PM, rama gottfried notifications@github.com wrote:

ok, so I just removed the flattening, and was able to keep it from crashing by commenting out just the subbundle memcpy part in the if(bs2){} block. (see below)

The result is that the address comes out with no values attached to it, which seems reasonable -- or maybe it should say something like: /foo : "subbundle" or something like that?

    if(bs1){
  long l = osc_bundle_s_getLen(bs1);
  char *p = osc_bundle_s_getPtr(bs1);

  memcpy(p + OSC_ID_SIZE, &timetag, sizeof(t_osc_timetag));

  for(int i = 0; i < nnestedbundles; i++){
      t_osc_bndl_s *bs2 = osc_bundle_u_serialize(nestedbundles[i]);
      if(bs2){
                 /*       
                           long ll = osc_bundle_s_getLen(bs2);
          char *pp = osc_bundle_s_getPtr(bs2);
          p = osc_mem_resize(p, l + ll);
          memcpy(p + l, pp, ll);
          l += ll;
                            */
          osc_bundle_s_deepFree(bs2);
      }
  }

  //if(x->bundle){
  omax_util_outletOSC(x->outlet, l, p);
      /*
  }else{
      t_osc_bndl_it_s *bit = osc_bndl_it_s_get(l, p);
      while(osc_bndl_it_s_hasNext(bit)){
          t_osc_msg_s *m = osc_bndl_it_s_next(bit);
          long ml = osc_message_s_getSize(m);
          char *mp = osc_message_s_getAddress(m);
          omax_util_outletOSC(x->outlet, ml, mp);
      }
      osc_bndl_it_s_destroy(bit);
  }
      */
  osc_bundle_s_deepFree(bs1);

} — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334772486, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdR_jtWCnzUbj-aQInyufkYgJTS4fks5spjo3gaJpZM4OFFX3.

[ramagottfried]

what about subsubbundles? do we need to do recursive bundle blobbing?

[maccallum]

yeah, all sub bundles become blobs

On Oct 6, 2017, at 4:42 PM, rama gottfried notifications@github.com wrote:

what about subsubbundles? do we need to do recursive bundle blobbing?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334775080, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdXXD5F56QFdjkxeH3rRfD11q9x9Sks5spjxdgaJpZM4OFFX3.

[ramagottfried]

ok cool, I think I might have that working now... i've never used blobs before...

if I understand the output correctly, it looks like it's all the raw bytes, including the subbundles so no recursion necessary?

[ramagottfried]

see: https://github.com/CNMAT/CNMAT-odot/commit/3504cd2c74a5bb2861a1e12aee3dc39a7fa6ad18

I also fixed the order of the o.downcast timetag (the fractional part was first)

[ramagottfried]

p.s. do you think this is what o.atomize should do also?

[maccallum]

I think o.downcast’s job should probably be to recursively process every sub bundle to convert the types to 1.0 types, including any sub sub bundles, etc.

There are other use cases for a tool that takes a bundle and makes a blob but leaves everything inside alone. The blob() function in o.expr /should/ do that, but I see it’s not behaving as I expect…

On Oct 6, 2017, at 5:10 PM, rama gottfried notifications@github.com wrote:

ok cool, I think I might have that working now... i've never used blobs before...

if I understand the output correctly, it looks like it might be including the subbundles so no recursion necessary?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334783111, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdTVit8zk4hzscr5N5cb6-cb9JlJEks5spkLWgaJpZM4OFFX3.

[maccallum]

No, because a blob isn’t a Max type

On Oct 6, 2017, at 5:21 PM, rama gottfried notifications@github.com wrote:

p.s. do you think this is what o.atomize should do also?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334786492, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdYvsstUsqHqmg0FNoi9_tyd25DQUks5spkWEgaJpZM4OFFX3.

[ramagottfried]

ok, that was what I was thinking too re: recursive downcasting. ... doesn't that mean flattening also?

[ramagottfried]

edit: just looked at the osc 1.0 spec again and see it says "Note this recursive definition: bundle may contain bundles." !

ok ok,... rtfm! : )

[maccallum]

Not really, o.flatten does a weird thing by concatenating addresses, but that’s not the same as converting a bundle to a blob.

On Oct 6, 2017, at 5:53 PM, rama gottfried notifications@github.com wrote:

ok, that was what I was thinking too re: recursive downcasting. ... doesn't that mean flattening also?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334795390, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdYFRD5mKXxbj15IhcSj7AujtXuJtks5spkzfgaJpZM4OFFX3.

[maccallum]

Just to be clear, we don’t want to convert a sub bundle into the kind of bundle that is described in the 1.0 spec—the one described in the spec isn’t bound to an address. What I’m suggesting is that every bundle becomes a blob.

On Oct 6, 2017, at 9:57 PM, rama gottfried notifications@github.com wrote:

edit: just looked at the osc 1.0 spec again and see it says "Note this recursive definition: bundle may contain bundles." !

ok ok,... rtfm! : )

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334854951, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdRLZCPJb8pw-B-ljOrn4pqIbHWerks5spoYhgaJpZM4OFFX3.

[ramagottfried]

ok, actually I think maybe that's what the version that was crashing was doing, appending subbundles unbound to addresses to the end of the output bundle. Which kind of makes sense, except that I don't think that odot can parse that kind of anonymous bundle array (just tried it with OpenSoundControl using the [ ... ] operators). should odot be able to parse OSC 1.0 bundle arrays?

but back to the blob idea, I'm not completely clear on what you're imagining.

for example, take a nested bundle situation:

{
    /foo : 0.1,
    /sub : {
        /bar : 0.3,
        /inner : {
            /steve : 0.0001
        }
    }
}

o.downcast should:

• first: go through all OSC messages and downcast 64bit to 32bit types.
• then: convert subbundles to blobs... so what does that mean for /inner? if we convert /sub to a blob /inner is included in that, so it would look like:

{
    /foo : 0.1,
    /sub : blob( <byte array containing /bar and /inner { /steve } > )
}

is that what you have in mind?

[maccallum]

I think there’s some confusion about bundles somehow becoming special just because they’re nested more than one level deep. A bundle is just a single piece of data with a type tag (‘.’), and a blob is a single piece of data with a type tag (‘b’). I’m suggesting that every bundle without exception gets converted into a blob. If a message /foo has a double, a bundle, and a blob, it’ll have a float and two blobs after o.downcast. In your example, /sub will have a blob. If someone decides to interpret that blob as a bundle, that bundle with have two messages: /bar with a float, and /inner with a blob that someone could interpret as a bundle.

On Oct 7, 2017, at 5:14 PM, rama gottfried notifications@github.com wrote:

ok, actually I think maybe that's what the version that was crashing was doing, appending subbundles unbound to addresses to the end of the output bundle. Which kind of makes sense, except that I don't think that odot can parse that kind of anonymous bundle array (just tried it with OpenSoundControl using the [ ... ] operators). should odot be able to parse OSC 1.0 bundle arrays?

but back to the blob idea, I'm not completely clear on what you're imagining.

for example, take a nested bundle situation:

{ /foo : 0.1, /sub : { /bar : 0.3, /inner : { /steve : 0.0001 } } } o.downcast should:

first: go through all OSC messages and downcast 64bit to 32bit types. then: convert subbundles to blobs... so what does that mean for /inner? if we convert /sub to a blob /inner is included in that, so it would look like: { /foo : 0.1, /sub : blob( <byte array containing /bar and /inner { /steve } > ) } is that what you have in mind?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334942085, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdV8foSpgP4hwXm5xVwCDf2nrU6Aoks5sp5U6gaJpZM4OFFX3.

[ramagottfried]

ok right, got it -- there should be no type . after the downcast, but b where . was, including all inner bundles.

[ramagottfried]

(i.e. no type . hidden in the blob)

[maccallum]

Yeah, that’s it. The bundle type, ‘.’, is basically odot’s way of distinguishing between a blob and a bundle, and so for OSC 1.0, the blob is the natural downcast, I think.

On Oct 7, 2017, at 7:05 PM, rama gottfried notifications@github.com wrote:

ok right, got it -- there should be no type . after the downcast, but b where . was, including all inner bundles.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-334950507, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdV2zV8OIVfvLHuQBW4Ir414w6g8Vks5sp69MgaJpZM4OFFX3.

[ramagottfried]

I have it working but there's a memory leak somewhere, still trying to find it.

I don't think this is the culprit in this case, but shouldn't osc_atom_u_clear check for and free memory for blobs also?

void osc_atom_u_clear(t_osc_atom_u *a)
{
    if(!a){
        return;
    }
    if(a->alloc && a->typetag == 's' && a->w.s){
        osc_mem_free(a->w.s);
        a->w.s = NULL;
    }else if(a->typetag == OSC_BUNDLE_TYPETAG){
        osc_bundle_u_free(a->w.bndl);
        a->w.bndl = NULL;
    }
    a->alloc = 0;
}

[ramagottfried]

ok, memory leak seems plugged after added free routines for type b in osc_atom_u_free and osc_atom_u_clear

[ramagottfried]

I moved the htonl conversion to be only for the header encoding, the timetag message I think should still be in host encoding so that it matches OSC-Timetag, otherwise things get all crazy.

[ramagottfried]

I think we can close this issue -- @maccallum feel free to reopen if you think it should be.

[maccallum]

If you send me a patch, I’ll test

On Oct 16, 2017, at 4:27 PM, rama gottfried notifications@github.com wrote:

Closed #340 https://github.com/CNMAT/CNMAT-odot/issues/340.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#event-1294948480, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdVgBoPb9RCRvXNaBY0mjS7A_ne4Dks5ss2ftgaJpZM4OFFX3.

[ramagottfried]

cool -- I added some test patches in the help patch (I should probably neaten the timetag tester out)

[maccallum]

Great—it’s checked in? I’ll put this on my todo list, but I won’t get to it until next week.

On Oct 16, 2017, at 4:54 PM, rama gottfried notifications@github.com wrote:

cool -- I added some test patches in the help patch (I should probably neaten the timetag tester out)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNMAT/CNMAT-odot/issues/340#issuecomment-336912379, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjjdefZmY4yUyMJmIO6EeKmMpQv6-5gks5ss24WgaJpZM4OFFX3.

[ramagottfried]

yep, it's there waiting for you impatiently : )

[equilet]

did this testing ever occur?

[maccallum]

This has been fixed and tested