Skip to content
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

First version of Intra only decoder #246

Merged
merged 9 commits into from May 24, 2019

Conversation

Projects
None yet
4 participants
@nijilk
Copy link
Contributor

commented May 10, 2019

  • Supports only I frames without loopfilter ( deblocking, cdef, self guided restoration filters not support)
  • Tiles not supported
  • Supports only Main profile 8bit 420
  • Tested only on Windows

Nijil K and others added some commits May 10, 2019

Nijil K
First version of Intra only decoder
- Supports only I frames without loopfilter ( deblocking, cdef, self guided restoration filters not support)
- Tiles not supported
- Supports only Main profile 8bit 420
- Tested only on Windows
@kylophone

This comment has been minimized.

Copy link
Collaborator

commented May 10, 2019

Source/Lib/Common/Codec/EbDefinitions.h.bak, assuming you didn't mean to commit that.

@@ -104,14 +186,14 @@ typedef struct EbSvtAv1DecConfiguration
* 0 = decodes from the start of the bitstream.
*
* Default is 0. */
uint64_t skip_frames;
uint64_t skipFrames;

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

snake_case ?


/* Maximum number of frames in the sequence to be decoded.
*
* 0 = decodes the full bitstream.
*
* Default is 0. */
uint64_t frames_to_be_decoded;
uint64_t framesToBeDecoded;

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

snake_case

@@ -134,6 +216,8 @@ typedef struct EbSvtAv1DecConfiguration

EbColorFormat max_color_format;


This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

extra space ?

if (!stop_after || in_frame < stop_after) {
return_error |= eb_svt_decode_frame(p_handle, buf,
(uint32_t)bytes_in_buffer);

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

extra space

AomCdfProb sgrproj_restore_cdf[CDF_SIZE(2)];
AomCdfProb y_mode_cdf[BlockSize_GROUPS][CDF_SIZE(INTRA_MODES)];
AomCdfProb uv_mode_cdf[CFL_ALLOWED_TYPES][INTRA_MODES]
aom_cdf_prob comp_ref_cdf[REF_CONTEXTS][FWD_REFS - 1][CDF_SIZE(2)];

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

types should not be snake_case


/**********************************************
* Intra Reference Samples Ctor
**********************************************/
EbErrorType intra_reference_samples_ctor(
IntraReferenceSamples **context_dbl_ptr)
EbErrorType IntraReferenceSamplesCtor(

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

snake_case

@@ -139,13 +172,13 @@ static int is_smooth(const MbModeInfo *mbmi, int plane) {
// detect that case.
if (is_inter_block(mbmi)) return 0;

const UvPredictionMode uv_mode = mbmi->uv_mode;
const UV_PredictionMode uv_mode = mbmi->uv_mode;

This comment has been minimized.

Copy link
@hassount

hassount May 12, 2019

Contributor

type CamelCase

nijilk and others added some commits May 13, 2019

@joelsole
Copy link
Contributor

left a comment

  • There are some structures like MV, intMV etc., repeated on the encoder and duplicated on the decoder
    • Is this an intermediate step with the goal of harmonization later on?
  • Style: no braces on single statements, ModeInfo_t
  • It seems some of the code is hard-coding 8bit and 420 (e.g., at svt_dec_out_buf)
    • Even if other formats are initially not supported, it would be good if the decoder code is more flexible from the start
  • Could you add more doxygen documentation? At least, for modules with external linkage and data structures 
in header files.
  • Be sure that all copyrights are properly set (e.g., is EbDecParseBlock.c correct?)
  • #if 0 and #if 1 should be removed
Nijil K
Fixed build issues in Linux and Mac
Some style changes as per review comments
Removed 8bit and 420 hard coding in svt_dec_out_buf
Removed most of the build warnings
@hassount
Copy link
Contributor

left a comment

Coding style can be revisited a little more especially brackets.
One test to make sure of would be that the encoder output does not change with this PR.

return EB_ErrorInsufficientResources; \
} \
else { \
svt_dec_memory_map[*(svt_dec_memory_map_index)].ptr_type = pointer_class; \

This comment has been minimized.

Copy link
@hassount

hassount May 16, 2019

Contributor

#242 has changed the way this is done. It would be nice to change it to the same linked list mode approach, that should save you some memory

This comment has been minimized.

Copy link
@nijilk

nijilk May 17, 2019

Author Contributor

Ok.
I will take care of that.

@joelsole

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

One test to make sure of would be that the encoder output does not change with this PR.

I did a short test and encoder output is the same. More extensive tests would be needed before merging.

Nijil K added some commits May 21, 2019

Nijil K
Fix Build issues after rebasing
Minor fixes in decoder
Line Ending issue in Mac is resolved
EbAV1FrameInfo *frame_info = (EbAV1FrameInfo*)malloc(sizeof(EbAV1FrameInfo));

if (config_ptr->skip_frames)
fprintf(stderr, "Skipping first %llud frames.\n", config_ptr->skip_frames);

This comment has been minimized.

Copy link
@joelsole

joelsole May 21, 2019

Contributor

This line is giving me a warning on Mac:

Source/App/DecApp/EbDecAppMain.c:183:54: warning: illegal character encoding in string literal
[-Winvalid-source-encoding]
fprintf(stderr, "Skipping first %lludframes.\n", config_ptr->skip_frames);
^~~~

Nijil K
@hassount

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

before merging, this PR, do we plan on addressing the existing decoder build warnings or do we plan to do another PR for such ?

@joelsole

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

before merging, this PR, do we plan on addressing the existing decoder build warnings or do we plan to do another PR for such ?

There are some warnings coming from master. Better if those are addressed on a separate PR.

@joelsole

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Let me try to address them.

@hassount hassount merged commit c7b108a into OpenVisualCloud:master May 24, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.