Skip to content
This repository has been archived by the owner on Aug 22, 2018. It is now read-only.

Ogg/opus encode/decode updates and two major bug fixes #33

Closed
wants to merge 30 commits into from

Conversation

mihui
Copy link
Collaborator

@mihui mihui commented Jun 26, 2016

  1. Simplified ogg/opus code
  2. Added continuous and interim result configurations
  3. Merged OggHelper and OpusHelper because they share code
  4. Fixed issue of recording never stops when the continuous option is on
  5. Adjusted (fixed) error handling code due to inconsistent error response from service, see https://developer.ibm.com/answers/questions/284164/500-forwarding-error-and-inconsistant-error-json-s/
  6. Removed unused header search paths
  7. Added microphone permission check, it is related to the issue [STT] No error when mic access was denied #34
  8. Expose token in case there is a need to set token outside of Xcode project (Sorry, previous comment is not good, I noticed that there is a requestToken method, but it is not in Android, so I will add it there)
  9. Added a flag for refreshing cached token to provide developer an option to prevent 401 error in advance

Michael added 23 commits June 20, 2016 23:04
…id transcription, merged STT result validations into one method, and introduced a basic SpeechToTextResult class

#import <Foundation/Foundation.h>
#import <AVFoundation/AVFoundation.h>
#import "CodecHeader.h"
Copy link
Collaborator

@nacho4d nacho4d Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mihui AFAIK OpusHelper.h is a public header. importing CodecHeader.h in a public header will required clients to import all headers inside and most of them are not public. Could this header to the .m file please? To do so all ogg type mentions will be need to be done in the .m file as class extension.

The less headers we expose the easier/better integration of this library will be I think

Copy link
Collaborator Author

@mihui mihui Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will get back to you. I have tried a little bit, but it seems making headers private or public does not change a thing. I will try something else tomorrow.

Copy link
Collaborator

@nacho4d nacho4d Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the new OggHelper.h could look as:

@interface OggHelper : NSObject
- (NSData *) getOggOpusHeader:(int) sampleRate opusHeader:(OpusHeader) header;
- (NSMutableData *) writePacket: (NSData*) data frameSize:(int) frameSize rate: (int) sampleRate;
@end

and put all ogg stuff in the OggHelper.m file

#import "OggHelper"
#import "ogg.h"

#import "opus_defines.h"
#import "opus_header.h"
#import "opus_types.h"

static void comment_init(char **comments, long* length, const char *vendor_string);
static void comment_pad(char **comments, long* length, int amount);
#define readint(buf, base) (((buf[base+3]<<24)&0xff000000)| \
((buf[base+2]<<16)&0xff0000)| \
((buf[base+1]<<8)&0xff00)| \
(buf[base]&0xff))
#define writeint(buf, base, val) do{ buf[base+3]=((val)>>24)&0xff; \
buf[base+2]=((val)>>16)&0xff; \
buf[base+1]=((val)>>8)&0xff; \
buf[base]=(val)&0xff; \
}while(0)

@interface OggHelper() {
    ogg_page oggPage;
    ogg_int64_t packetCount;
    ogg_int16_t granulePos;
    ogg_stream_state streamState;
}
@end

@implementation OggHelper
...
@end

However I just realized that now there is OpusHeader in the .h file so maybe is not going to be easy to hide ogg.h and friends. Maybe not possible now...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@mihui mihui Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OggHelper reference has been deleted in my repo, I migrated those helpers into one OpusHelper because they are all related to Opus, maybe I should delete the file: d2a9b26

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, Right now is not possible to merge this PR. Can you rebase or merge this branch on master and push it again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just found out it was not easy to merge, and I did some optimizations according to the header thing, not only the codec header. Now there are only 8 headers in public in my repo. I will do some updates for merging branches later.

@mihui
Copy link
Collaborator Author

mihui commented Jul 7, 2016

Unable to merge, closing.

@mihui mihui closed this Jul 7, 2016
@nacho4d
Copy link
Collaborator

nacho4d commented Jul 7, 2016

😢

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants