New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added ability to demux CEA-608 and CEA-708 closed captions #85
base: master
Are you sure you want to change the base?
Conversation
BlakeOrth
commented
Nov 26, 2018
- Added codec and box parsing for c608 and c708 QTFF box types
- Added QTFF closed captioning media description type (clcp) handler
- Added a generic summary for media samples that are not audio or video
- Added codec and box parsing for c608 and c708 QTFF box types - Added QTFF closed captioning media description type (clcp) handler - Added a generic summary for media samples that are not audio or video
Ping |
fixed segfault when parsing clli and mdcv boxes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note L-SMASH doesn't allow merge commits.
Not sure if @VFR-maniac has been too busy or what, so I took a look.
Aside from the few nits mentioned, it seems pretty reasonable to me, in my opinion.
@@ -283,10 +283,18 @@ static int isom_initialize_structured_codec_specific_data( lsmash_codec_specific | |||
specific->size = sizeof(lsmash_qt_audio_channel_layout_t); | |||
specific->destruct = lsmash_free; | |||
break; | |||
case LSMASH_CODEC_SPECIFIC_DATA_TYPE_ISOM_RTP_HINT_COMMON: | |||
case LSMASH_CODEC_SPECIFIC_DATA_TYPE_ISOM_RTP_HINT_COMMON : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray change should be a separate commit.
specific->size = sizeof(lsmash_qt_content_light_level_info_t); | ||
specific->destruct = lsmash_free; | ||
break; | ||
case LSMASH_CODEC_SPECIFIC_DATA_TYPE_QT_VIDEO_MASTERING_DISPLAY_COLOR_VOLUME : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit seems cherry-pickable outside of the captions stuff.
@kodabb ping, can you look at this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the seg-fault was impacting our usage of the library and I wasn't sure how long it was going to take to get the changes into the upstream. I'm happy to either:
- rebase/fast-forward this bug fix to get rid of the merge commit
or - nuke the bug fix and make a separate PR
Let me know how we'd like to proceed.
static int isom_setup_closed_captioning_description( isom_stsd_t *stsd, lsmash_summary_t *summary ) | ||
{ | ||
lsmash_codec_type_t sample_type = summary->sample_type; | ||
if( lsmash_check_box_type_identical( sample_type, QT_CODEC_TYPE_C708_CAPTION ) || lsmash_check_box_type_identical( sample_type, QT_CODEC_TYPE_C608_CAPTION ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line breaks needed.