Adddate: 2009-09-25 17:08:20
Overall, looks pretty good! A few comments:
* In the README.TXT where it lists the elements, there doesn't seem to be any
obvious ordering (other than maybe the order of their creation). Would you mind
cleaning this up, instead of just adding your new element at the end? Maybe put
all video, then audio, then imaging elements, and for each put decode before
encode, and xdm 1.x before xdm0.9. Like this:
(you get the picture)
We may also want to order the elements like this in gstticodecplugin.c as
* I don't know that the "Based on" comment in your header is
really necessary. All the elements use the same basic template (Auddec1 was
based on Auddec which was based on Viddec, etc.).
* In gst_tiaudenc1_set_source_caps you are always assuming that AAC will be
used, without ever checking what codec is being used. It's true that we only
support the AAC mime type for now, but you should check to make sure your codec
matches as well (see TIVidenc1 for reference). Both your element and TIVidenc1
are lacking code to fail caps negotiation if the codec being used doesn't match.
Right now in this case we will set the caps to an uninitalized value, which is
bad. We should probably fix this.
* In the init function, you used a tab in this line:
" audenc1->bitrate = 0;"
Please remove the tab.
Adddate: 2009-09-25 17:12:24
Sorry Chase, the previous follow-up was just for your first patch.
In your second patch, you changed the timeout to 6 but didn't update the block
comment above the code that says "two":
/* Try once a second to re-claim the next buffer, up to two
Also, how did you decide 6 was the best number. Seems a little excessive.
Otherwise I'm ok with this.
Adddate: 2009-09-25 17:17:05
Also, I might clarify your ChangeLog a little on Patch 2 -- I would say
"increased the timeout" or "changed to wait
longer". Just "changed" doesn't provide a lot of
insight into the change.
On to patch 3...
No issue with patch 3. Looks good!
Adddate: 2009-09-26 17:52:05
Chase , thanks patches looks pretty good.
Adddate: 2009-09-28 10:10:40
Updated patches to incorporate suggestions from Don.
1. Re-ordered elements in README.txt (not in the element registration. That
will be another patch)
2. Based on comments were to give credit to others work that was used.
3. Changed source caps negotiation to check the codec type and fail if it is an
4. Removed tab
5. Changed comment for timeout. 6 was found by testing various delay
6. Clarified Changelog
Adddate: 2009-09-28 11:35:37
Thanks for updating. Only this one little nit left:
In gst_tiaudenc1_set_source_caps, your GST_ELEMENT_ERROR line wraps. Please
break lines at 80 columns.
Adddate: 2009-09-29 13:01:55
Patches looks good,
except following point along with Don's previous comment
* getting 3 warnings , since in gst_tiaudenc1_init_env (), the GST_LOG () for
bitrate, channels and samplefreq %ld (long int) is used , instead of %d
Adddate: 2009-09-30 08:45:10
Updated patches to fix issues reported by Don and Kapil and rebase against
latest trunk (rev 377).
Can you verify that the warnings went away?
Adddate: 2009-10-01 09:11:11
Seems good to me.
Adddate: 2009-10-05 08:52:04
rebased patches to trunk rev 392 for checking in.
Adddate: 2009-10-05 13:28:01
I am trying to update the feature matrix at http://gstreamer.ti.com. You patch
comments only mention DM6467, but DM6446 and DM355 also use DVSDK 2.00.00.22.
Are they also supported, or just DM6467?
Adddate: 2009-10-05 13:35:06
DM6467 is the only device with xDM 1.x audio encode codecs.
Also, generally we don't update the matrix on the front page until the feature
has been included in a release. i.e. 1.1 release.
Adddate: 2009-10-05 13:35:18
In theory it should work with xDM 1.x audio encoders on any platform, but DM6467
is the only one that acutally has a codec in the DVSDK that we can test with. I
would probably mark other platforms as purple for "untested".
Chase may want to comment more.